Implement IaddCin, IaddCout, and IaddCarry for Cranelift interpreter

[dheaton-arm]

Implemented the following Opcodes for the Cranelift interpreter:

• IaddCin to add two scalar integers with an input carry flag.
• IaddCout to add two scalar integers and report overflow with the carry flag.
• IaddCarry to add two scalar integers with an input carry flag, reporting overflow with the output carry flag.

Copyright (c) 2021, Arm Limited

[afonso360]

It looks like these instructions don't work at all in any of the backends. This is slightly concerning to me because I suspect that they may be legacy backend only instructions which will be deprecated soon. (is this the case @cfallin?).

That being said, and assuming that they are a TODO item on the backends, I think we should move these tests to the runtests folder, even if they only run in the interpreter. Reason being, that eventually we will implement them, and these tests are a lot easier to find there.

One of the things that I've been working towards is removing the interpreter folder and moving all test cases to the runtests folder. I think that the distinction isn't very meaningful. We already have some tests there that only work in the interpreter, but we never explicitly discussed this. Thoughts @cfallin, @abrown ?

[abrown]

I dug into the history of these instructions: iadd_carry, iadd_cin, and iadd_cout are not used by the Wasm-to-CLIF converter in code_translator.rs and do not appear in any of the ISA-specific lowering code in the new backend (e.g., cranelift/codegen/src/isa/*). They were added by @ryzokuken almost two years ago in bytecodealliance/cranelift#1005 and, unless they or @bjorn3 are still using these instructions, I propose we remove them. (@afonso360, I'm fine with moving stuff into the runtests folder).

[abrown]

The problem with this is that I hate to get rid of good code: thanks @dheaton-arm and @afonso360 for fleshing out the interpreter! We've talked before about cleaning up the CLIF opcode space and that probably should have happened before we jumped in on the interpreter, sorry; @cfallin can correct me if I'm wrong but I would think any CLIF opcode here or anything starting with X86... is a likely candidate for removal?

[bjorn3]

unless they or @bjorn3 are still using these instructions

I currently don't use them as backend support is bad, but if backend support is implemented I will want to use at least the _cout variants to detect overflows efficiently.

https://github.com/bjorn3/rustc_codegen_cranelift/blob/9f5b52045c928157b12bec1d670e220fc7597375/src/num.rs#L198-L225

The _carry and I think _cin variants are necessary to efficiently implement certain llvm intrinsics used by core::arch intrinsics that are stabilized:

https://github.com/bjorn3/rustc_codegen_cranelift/blob/9f5b52045c928157b12bec1d670e220fc7597375/src/intrinsics/llvm.rs#L145-L181

[bjorn3]

@cfallin can correct me if I'm wrong but I would think any CLIF opcode here or anything starting with X86... is a likely candidate for removal?

The _imm opcodes are currently legalized to non-_imm variants. I use the _imm variants extensively in cg_clif as they are much more readable both in the cg_clif source and in textual clif ir form. The if/ff variants of instructions should be removed though IMO. Just like the x86_ instructions.

[cfallin]

Sorry for the delay in responding here -- first of all, yes, we do need to clean up the opcode space!

The X86... opcodes are going away for sure -- that's uncontroversial I think. For the remainder, I think there are two fundamental questions here (to refine @afonso360 's excellent summary a bit):

• Do we have "convenience ops" like iadd_imm that are combinations of other instructions?
• How do we represent carry/borrow flags?

On the first point, we've had discussions in the past about "combo ops" and settled on mostly not having them, unless we have some complex behavior that involves more than ~2 instructions and really should stay bundled for easier lowering. (E.g. expanding to 9 opcodes then pattern-matching that back to a known machine instruction sequence for the whole group is suboptimal.)

We've sort of accepted the _imm variants for now but I do think they actually fall under the same reasoning and as such it would be better not to include them in the opcode space. That doesn't mean we can't have builder methods that remain with the same signature; these methods would just generate two opcodes (iconst and iadd for iadd_imm, for example). We don't have a mechanism for that in the meta-crate today but it seems like it wouldn't be too bad. The other counterargument that occurs to me is efficiency/density in the IR -- the separate instruction format packs the immediate into the same instruction. And there is readability as @bjorn3 mentions above. However pulling it out potentially has advantages too, e.g. for GVN. The major upside is that we don't have to implement _imm variants in every backend/interpreter/analysis that consumes CLIF, and that seems worth it to me. Thoughts?

The other question is how to handle carries. With pattern-matching, we can handle either the carry-as-bool or carry-as-part-of-flags with about the same effort. I actually don't like the "flags as a special value" approach all that much -- it has unique constraints, in that only one flags value can be live at a time, and is sort of a relic from the encodings approach to lowering. So from first principles I'd prefer carry-as-bool. For the same reasons I'd also prefer icmp over ifcmp. But that's a nontrivial refactoring, and it doesn't have to happen right away. It looks like @bjorn3 and @afonso360 agree here.

So back to the subject of this PR, I think that means (if others agree) that we should keep the cin/cout/carry variants here, as they seem to be the cleaner option and I'd want to move in that direction. Then at some point we can refactor the rest of the compiler to use them too. (Ideally after we have a DSL to describe instruction lowering, making such refactoring "trivial", but that's a separate push!) In the meantime, it's fine IMHO to have the ops working only in the interpreter as long as we have tests that describe their behavior.

Thoughts?

[abrown]

My opinion: sounds like @bjorn3 can use these instructions so let's resolve any comments and merge this! Re: removing _imm variants, I agree with the approach @cfallin outlined to remove the opcodes--that one sounds like an implementable issue we could create. Re: flags-to-bool, I am not sure I understand all the effects of this, but it sounds reasonable and could be a separate issue as well.

[afonso360]

sounds like @bjorn3 can use these instructions so let's resolve any comments and merge this!

Agreed!

---

I tend to agree about removing _imm.

I also think that with a builder the const is probably always going to be above the op so the readability lost is somewhat minimized.

Some concerns are that iadd_imm has a special meaning in global values, but that probably should be discussed in the _imm remove issue.

Re: flags-to-bool, I am not sure I understand all the effects of this, but it sounds reasonable and could be a separate issue as well.

Yeah, I prefer carry-as-bool as well, but I don't fully understand where else this change is going to impact

[cfallin]

OK cool, I created #3249 and #3250 to track these simplifications.

[afonso360]

So, after all this, @dheaton-arm Could you please move the files to runtests and resolve the rest of the comments, and we'll merge this?

Thanks!

[afonso360]

LGTM! Thanks!

Edit: Whoops, forgot about moving tests to the runtest folder, would you be able to do that please?