wasmtime / PR #3233 Implement `IaddCin`, `IaddCout`, and ... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #3233 Implement `IaddCin`, `IaddCout`, and ...

Wasmtime GitHub notifications bot (Aug 24 2021 at 11:25):

dheaton-arm opened PR #3233 from implement-iaddc to main:

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

Wasmtime GitHub notifications bot (Aug 25 2021 at 11:51):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 11:51):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 11:51):

afonso360 created PR review comment:

Would it be possible to also add tests for i64?

Wasmtime GitHub notifications bot (Aug 25 2021 at 11:51):

afonso360 created PR review comment:

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 ?

Wasmtime GitHub notifications bot (Aug 25 2021 at 11:51):

afonso360 created PR review comment:

            let carry = Value::lt(&sum, &arg(0)?)? && Value::lt(&sum, &arg(1)?)?;

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:22):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:22):

abrown created PR review comment:

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 https://github.com/bytecodealliance/cranelift/pull/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).

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:27):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:27):

abrown created PR review comment:

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?

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:28):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:28):

bjorn3 created PR review comment:

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 stably exported simd intrinsics:

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

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:29):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:33):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:33):

bjorn3 created PR review comment:

@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.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:33):

bjorn3 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:42):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 16:42):

abrown created PR review comment:

I guess the decision should be framed as an "all or nothing:" either we remove them from the interpreter and CLIF or we implement them in the interpreter AND the backends. E.g., the _imm variants are implemented in the interpreter and it wouldn't be too difficult to add support in the backends.

Wasmtime GitHub notifications bot (Aug 25 2021 at 17:32):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 25 2021 at 17:32):

afonso360 created PR review comment:

I think we have 4 categories of opcodes here:

op_imm: These already work on the backends, and in the interpreter, and it looks like they are useful.

op_cin/op_cout/op_carry: These use a bool to signal a carry / borrow. They only work in the interpreter, but it sounds like there are uses for them if they worked in the backends.

op_ifcin/op_ifcout/op_ifcarry: These use the cpu flags type to achieve essentially what their non if version does.

X86 prefixed: These are legacy, and we know they are going away in #3009.

I agree with @bjorn3 about removing the if flag typed instructions. But that may be because I don't really understand the CPU flags type, or why we need it.

the _imm variants are implemented in the interpreter and it wouldn't be too difficult to add support in the backends.

I'm not sure we need to implement them in the backends separately, the current approach of transforming them into op + const probably saves some work. However, legalization was mentioned, and I think that is also going away in #3009 right?

Wasmtime GitHub notifications bot (Aug 25 2021 at 17:37):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 25 2021 at 18:08):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 25 2021 at 18:10):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 26 2021 at 16:35):

cfallin created PR review comment:

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?

Wasmtime GitHub notifications bot (Aug 26 2021 at 16:35):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2021 at 16:57):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2021 at 16:57):

abrown created PR review comment:

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.

Wasmtime GitHub notifications bot (Aug 26 2021 at 17:08):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2021 at 17:08):

afonso360 created PR review comment:

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 the readability lost is probably 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

Wasmtime GitHub notifications bot (Aug 26 2021 at 17:12):

afonso360 edited PR review comment.

Wasmtime GitHub notifications bot (Aug 26 2021 at 17:22):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2021 at 17:22):

cfallin created PR review comment:

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

Wasmtime GitHub notifications bot (Aug 26 2021 at 17:41):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 26 2021 at 17:41):

afonso360 created PR review comment:

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!

Wasmtime GitHub notifications bot (Aug 31 2021 at 09:42):

dheaton-arm updated PR #3233 from implement-iaddc to main.

Wasmtime GitHub notifications bot (Aug 31 2021 at 09:47):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2021 at 09:47):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2021 at 09:53):

afonso360 submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2021 at 10:27):

dheaton-arm updated PR #3233 from implement-iaddc to main.

Wasmtime GitHub notifications bot (Aug 31 2021 at 16:29):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 31 2021 at 16:29):

abrown merged PR #3233.

Last updated: Nov 22 2024 at 17:03 UTC