Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement IaddCin, IaddCout, and IaddCarry for Cranelift interpreter #3233

Merged
merged 3 commits into from
Aug 31, 2021
Merged

Implement IaddCin, IaddCout, and IaddCarry for Cranelift interpreter #3233

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f95d257
Implement `IaddCin`, `IaddCout`, and `IaddCarry` for Cranelift interp…
dheaton-arm Aug 16, 2021
e99cc57
Simplify carry check + add i64 `IaddCarry` tests
dheaton-arm Aug 25, 2021
565d5c9
Move tests to `runtests`
dheaton-arm Aug 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 73 additions & 0 deletions cranelift/filetests/filetests/interpreter/iaddcarry.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
test interpret
Copy link
Contributor

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 ?

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor

@bjorn3 bjorn3 Aug 25, 2021 •
edited
Loading

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.

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

Copy link
Contributor

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.

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor

@afonso360 afonso360 Aug 26, 2021 •
edited
Loading

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

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!


function %iaddcarry_i8_v(i8, i8, b1) -> i8 {
block0(v0: i8, v1: i8, v2: b1):
v3, v4 = iadd_carry v0, v1, v2
return v3
}
; run: %iaddcarry_i8_v(0, 1, true) == 2
; run: %iaddcarry_i8_v(0, 1, false) == 1
; run: %iaddcarry_i8_v(100, 27, true) == -128
; run: %iaddcarry_i8_v(100, 27, false) == 127
; run: %iaddcarry_i8_v(127, 127, true) == -1
; run: %iaddcarry_i8_v(127, 127, false) == -2

function %iaddcarry_i8_c(i8, i8, b1) -> b1 {
block0(v0: i8, v1: i8, v2: b1):
v3, v4 = iadd_carry v0, v1, v2
return v4
}
; run: %iaddcarry_i8_c(0, 1, true) == false
; run: %iaddcarry_i8_c(0, 1, false) == false
; run: %iaddcarry_i8_c(100, 27, true) == true
; run: %iaddcarry_i8_c(100, 27, false) == false
; run: %iaddcarry_i8_c(127, 127, true) == true
; run: %iaddcarry_i8_c(127, 127, false) == true

function %iaddcarry_i16_v(i16, i16, b1) -> i16 {
block0(v0: i16, v1: i16, v2: b1):
v3, v4 = iadd_carry v0, v1, v2
return v3
}
; run: %iaddcarry_i16_v(0, 1, true) == 2
; run: %iaddcarry_i16_v(0, 1, false) == 1
; run: %iaddcarry_i16_v(100, 27, true) == 128
; run: %iaddcarry_i16_v(100, 27, false) == 127
; run: %iaddcarry_i16_v(32000, 767, true) == -32768
; run: %iaddcarry_i16_v(32000, 767, false) == 32767

function %iaddcarry_i16_c(i16, i16, b1) -> b1 {
block0(v0: i16, v1: i16, v2: b1):
v3, v4 = iadd_carry v0, v1, v2
return v4
}
; run: %iaddcarry_i16_c(0, 1, true) == false
; run: %iaddcarry_i16_c(0, 1, false) == false
; run: %iaddcarry_i16_c(100, 27, true) == false
; run: %iaddcarry_i16_c(100, 27, false) == false
; run: %iaddcarry_i16_c(32000, 767, true) == true
; run: %iaddcarry_i16_c(32000, 767, false) == false

function %iaddcarry_i32_v(i32, i32, b1) -> i32 {
block0(v0: i32, v1: i32, v2: b1):
v3, v4 = iadd_carry v0, v1, v2
return v3
}
; run: %iaddcarry_i32_v(0, 1, true) == 2
; run: %iaddcarry_i32_v(0, 1, false) == 1
; run: %iaddcarry_i32_v(100, 27, true) == 128
; run: %iaddcarry_i32_v(100, 27, false) == 127
; run: %iaddcarry_i32_v(2000000000, 147483647, true) == -2147483648
; run: %iaddcarry_i32_v(2000000000, 147483647, false) == 2147483647

function %iaddcarry_i32_c(i32, i32, b1) -> b1 {
block0(v0: i32, v1: i32, v2: b1):
v3, v4 = iadd_carry v0, v1, v2
return v4
}
; run: %iaddcarry_i32_c(0, 1, true) == false
; run: %iaddcarry_i32_c(0, 1, false) == false
; run: %iaddcarry_i32_c(100, 27, true) == false
; run: %iaddcarry_i32_c(100, 27, false) == false
; run: %iaddcarry_i32_c(2000000000, 147483647, true) == true
; run: %iaddcarry_i32_c(2000000000, 147483647, false) == false
dheaton-arm marked this conversation as resolved.
Show resolved Hide resolved
48 changes: 48 additions & 0 deletions cranelift/filetests/filetests/interpreter/iaddcin.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
test interpret

function %iaddcin_i8(i8, i8, b1) -> i8 {
block0(v0: i8, v1: i8, v2: b1):
v3 = iadd_cin v0, v1, v2
return v3
}
; run: %iaddcin_i8(0, 1, true) == 2
; run: %iaddcin_i8(0, 1, false) == 1
; run: %iaddcin_i8(100, 27, true) == -128
; run: %iaddcin_i8(100, 27, false) == 127

function %iaddcin_i16(i16, i16, b1) -> i16 {
block0(v0: i16, v1: i16, v2: b1):
v3 = iadd_cin v0, v1, v2
return v3
}
; run: %iaddcin_i16(0, 1, true) == 2
; run: %iaddcin_i16(0, 1, false) == 1
; run: %iaddcin_i16(100, 27, true) == 128
; run: %iaddcin_i16(100, 27, false) == 127
; run: %iaddcin_i16(32000, 767, true) == -32768
; run: %iaddcin_i16(32000, 767, false) == 32767

function %iaddcin_i32(i32, i32, b1) -> i32 {
block0(v0: i32, v1: i32, v2: b1):
v3 = iadd_cin v0, v1, v2
return v3
}
; run: %iaddcin_i32(0, 1, true) == 2
; run: %iaddcin_i32(0, 1, false) == 1
; run: %iaddcin_i32(100, 27, true) == 128
; run: %iaddcin_i32(100, 27, false) == 127
; run: %iaddcin_i32(2000000000, 147483647, true) == -2147483648
; run: %iaddcin_i32(2000000000, 147483647, false) == 2147483647


function %iaddcin_i64(i64, i64, b1) -> i64 {
block0(v0: i64, v1: i64, v2: b1):
v3 = iadd_cin v0, v1, v2
return v3
}
; run: %iaddcin_i64(0, 1, true) == 2
; run: %iaddcin_i64(0, 1, false) == 1
; run: %iaddcin_i64(100, 27, true) == 128
; run: %iaddcin_i64(100, 27, false) == 127
; run: %iaddcin_i64(2000000000, 147483647, true) == 2147483648
; run: %iaddcin_i64(2000000000, 147483647, false) == 2147483647
87 changes: 87 additions & 0 deletions cranelift/filetests/filetests/interpreter/iaddcout.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
test interpret

function %iaddcout_i8_v(i8, i8) -> i8 {
block0(v0: i8, v1: i8):
v2, v3 = iadd_cout v0, v1
return v2
}
; run: %iaddcout_i8_v(0, 1) == 1
; run: %iaddcout_i8_v(100, 27) == 127
; run: %iaddcout_i8_v(100, -20) == 80
; run: %iaddcout_i8_v(100, 28) == -128

function %iaddcout_i8_c(i8, i8) -> b1 {
block0(v0: i8, v1: i8):
v2, v3 = iadd_cout v0, v1
return v3
}
; run: %iaddcout_i8_c(0, 1) == false
; run: %iaddcout_i8_c(100, 27) == false
; run: %iaddcout_i8_c(100, -20) == false
; run: %iaddcout_i8_c(100, 28) == true

function %iaddcout_i16_v(i16, i16) -> i16 {
block0(v0: i16, v1: i16):
v2, v3 = iadd_cout v0, v1
return v2
}
; run: %iaddcout_i16_v(0, 1) == 1
; run: %iaddcout_i16_v(100, 27) == 127
; run: %iaddcout_i16_v(100, 28) == 128
; run: %iaddcout_i16_v(32000, 767) == 32767
; run: %iaddcout_i16_v(32000, 768) == -32768

function %iaddcout_i16_c(i16, i16) -> b1 {
block0(v0: i16, v1: i16):
v2, v3 = iadd_cout v0, v1
return v3
}
; run: %iaddcout_i16_c(0, 1) == false
; run: %iaddcout_i16_c(100, 27) == false
; run: %iaddcout_i16_c(100, 28) == false
; run: %iaddcout_i16_c(32000, 767) == false
; run: %iaddcout_i16_c(32000, 768) == true

function %iaddcout_i32_v(i32, i32) -> i32 {
block0(v0: i32, v1: i32):
v2, v3 = iadd_cout v0, v1
return v2
}
; run: %iaddcout_i32_v(0, 1) == 1
; run: %iaddcout_i32_v(100, 27) == 127
; run: %iaddcout_i32_v(100, 28) == 128
; run: %iaddcout_i32_v(2000000000, 147483647) == 2147483647
; run: %iaddcout_i32_v(2000000000, 147483648) == -2147483648

function %iaddcout_i32_c(i32, i32) -> b1 {
block0(v0: i32, v1: i32):
v2, v3 = iadd_cout v0, v1
return v3
}
; run: %iaddcout_i32_c(0, 1) == false
; run: %iaddcout_i32_c(100, 27) == false
; run: %iaddcout_i32_c(100, 28) == false
; run: %iaddcout_i32_c(2000000000, 147483647) == false
; run: %iaddcout_i32_c(2000000000, 147483648) == true

function %iaddcout_i64_v(i64, i64) -> i64 {
block0(v0: i64, v1: i64):
v2, v3 = iadd_cout v0, v1
return v2
}
; run: %iaddcout_i64_v(0, 1) == 1
; run: %iaddcout_i64_v(100, 27) == 127
; run: %iaddcout_i64_v(100, 28) == 128
; run: %iaddcout_i64_v(2000000000, 147483647) == 2147483647
; run: %iaddcout_i64_v(2000000000, 147483648) == 2147483648

function %iaddcout_i64_c(i64, i64) -> b1 {
block0(v0: i64, v1: i64):
v2, v3 = iadd_cout v0, v1
return v3
}
; run: %iaddcout_i64_c(0, 1) == false
; run: %iaddcout_i64_c(100, 27) == false
; run: %iaddcout_i64_c(100, 28) == false
; run: %iaddcout_i64_c(2000000000, 147483647) == false
; run: %iaddcout_i64_c(2000000000, 147483648) == false
24 changes: 21 additions & 3 deletions cranelift/interpreter/src/step.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,11 +454,29 @@ where
Opcode::UremImm => binary_unsigned_can_trap(Value::rem, arg(0)?, imm())?,
Opcode::SremImm => binary_can_trap(Value::rem, arg(0)?, imm_as_ctrl_ty()?)?,
Opcode::IrsubImm => binary(Value::sub, imm_as_ctrl_ty()?, arg(0)?)?,
Opcode::IaddCin => unimplemented!("IaddCin"),
Opcode::IaddCin => choose(
Value::into_bool(arg(2)?)?,
Value::add(Value::add(arg(0)?, arg(1)?)?, Value::int(1, ctrl_ty)?)?,
Value::add(arg(0)?, arg(1)?)?,
),
Opcode::IaddIfcin => unimplemented!("IaddIfcin"),
Opcode::IaddCout => unimplemented!("IaddCout"),
Opcode::IaddCout => {
let sum = Value::add(arg(0)?, arg(1)?)?;
let mut carry = false;
if Value::lt(&sum, &arg(0)?)? && Value::lt(&sum, &arg(1)?)? {
carry = true;
}
dheaton-arm marked this conversation as resolved.
Show resolved Hide resolved
assign_multiple(&[sum, Value::bool(carry, types::B1)?])
}
Opcode::IaddIfcout => unimplemented!("IaddIfcout"),
Opcode::IaddCarry => unimplemented!("IaddCarry"),
Opcode::IaddCarry => {
let mut sum = Value::add(arg(0)?, arg(1)?)?;
if Value::into_bool(arg(2)?)? {
sum = Value::add(sum, Value::int(1, ctrl_ty)?)?
}
let carry = Value::lt(&sum, &arg(0)?)? && Value::lt(&sum, &arg(1)?)?;
assign_multiple(&[sum, Value::bool(carry, types::B1)?])
}
Opcode::IaddIfcarry => unimplemented!("IaddIfcarry"),
Opcode::IsubBin => unimplemented!("IsubBin"),
Opcode::IsubIfbin => unimplemented!("IsubIfbin"),
Expand Down