Stream: git-wasmtime

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


view this post on Zulip 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:

Copyright (c) 2021, Arm Limited

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 11:51):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 11:51):

afonso360 submitted PR review.

view this post on Zulip 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?

view this post on Zulip 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 ?

view this post on Zulip 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)?)?;

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 16:22):

abrown submitted PR review.

view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 16:27):

abrown submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 16:28):

bjorn3 submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 16:29):

bjorn3 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 16:33):

bjorn3 submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 16:33):

bjorn3 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 16:42):

abrown submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 17:32):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 17:32):

afonso360 created PR review comment:

I think we have 4 categories of opcodes here:

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?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 17:37):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 18:08):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 25 2021 at 18:10):

afonso360 edited PR review comment.

view this post on Zulip 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):

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?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2021 at 16:35):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2021 at 16:57):

abrown submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2021 at 17:08):

afonso360 submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2021 at 17:12):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2021 at 17:22):

cfallin submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2021 at 17:41):

afonso360 submitted PR review.

view this post on Zulip 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!

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 09:42):

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

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 09:47):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 09:47):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 09:53):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 10:27):

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

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 16:29):

abrown submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 16:29):

abrown merged PR #3233.


Last updated: Dec 23 2024 at 13:07 UTC