-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement IaddCin
, IaddCout
, and IaddCarry
for Cranelift interpreter
#3233
Conversation
…reter 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
@@ -0,0 +1,73 @@ | |||
test interpret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
The _carry
and I think _cin
variants are necessary to efficiently implement certain llvm intrinsics used by core::arch
intrinsics that are stabilized:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
Copyright (c) 2021, Arm Limited
Copyright (c) 2021, Arm Limited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
Edit: Whoops, forgot about moving tests to the runtest folder, would you be able to do that please?
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