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

cranelift: Sign extend immediates in instructions that embed them. #4602

Merged
merged 4 commits into from
Aug 15, 2022
Merged

cranelift: Sign extend immediates in instructions that embed them. #4602

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2dd1d08
cranelift: Sign extend immediates in instructions that embed them.
afonso360 Aug 3, 2022
76b3047
cranelift: Clarify imm instruction behaviour
afonso360 Aug 9, 2022
61ccd93
cranelift: Deduplicate imm_const
afonso360 Aug 15, 2022
c8f720b
cranelift: zero extend logical imm ops
afonso360 Aug 15, 2022
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
26 changes: 21 additions & 5 deletions cranelift/codegen/meta/src/shared/instructions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1799,7 +1799,7 @@ pub(crate) fn define(
Compare scalar integer to a constant.

This is the same as the `icmp` instruction, except one operand is
an immediate constant.
a sign extended 64 bit immediate constant.

This instruction can only compare scalars. Use `icmp` for
lane-wise vector comparisons.
Expand Down Expand Up @@ -2058,7 +2058,7 @@ pub(crate) fn define(
r#"
Add immediate integer.

Same as `iadd`, but one operand is an immediate constant.
Same as `iadd`, but one operand is a sign extended 64 bit immediate constant.

Polymorphic over all scalar integer types, but does not support vector
types.
Expand All @@ -2075,6 +2075,8 @@ pub(crate) fn define(
r#"
Integer multiplication by immediate constant.

Same as `imul`, but one operand is a sign extended 64 bit immediate constant.

Polymorphic over all scalar integer types, but does not support vector
types.
"#,
Expand All @@ -2090,6 +2092,8 @@ pub(crate) fn define(
r#"
Unsigned integer division by an immediate constant.

Same as `udiv`, but one operand is a sign extended 64 bit immediate constant.

This operation traps if the divisor is zero.
"#,
&formats.binary_imm64,
Expand All @@ -2104,6 +2108,8 @@ pub(crate) fn define(
r#"
Signed integer division by an immediate constant.

Same as `sdiv`, but one operand is a sign extended 64 bit immediate constant.

This operation traps if the divisor is zero, or if the result is not
representable in `B` bits two's complement. This only happens
when `x = -2^{B-1}, Y = -1`.
Expand All @@ -2120,6 +2126,8 @@ pub(crate) fn define(
r#"
Unsigned integer remainder with immediate divisor.

Same as `urem`, but one operand is a sign extended 64 bit immediate constant.

This operation traps if the divisor is zero.
"#,
&formats.binary_imm64,
Expand All @@ -2134,6 +2142,8 @@ pub(crate) fn define(
r#"
Signed integer remainder with immediate divisor.

Same as `srem`, but one operand is a sign extended 64 bit immediate constant.

This operation traps if the divisor is zero.
"#,
&formats.binary_imm64,
Expand All @@ -2147,6 +2157,8 @@ pub(crate) fn define(
"irsub_imm",
r#"
Immediate reverse wrapping subtraction: `a := Y - x \pmod{2^B}`.

The immediate operand is a sign extended 64 bit constant.

Also works as integer negation when `Y = 0`. Use `iadd_imm`
with a negative immediate operand for the reverse immediate
Expand Down Expand Up @@ -2550,7 +2562,7 @@ pub(crate) fn define(
r#"
Bitwise and with immediate.

Same as `band`, but one operand is an immediate constant.
Same as `band`, but one operand is a sign extended 64 bit immediate constant.

Polymorphic over all scalar integer types, but does not support vector
types.
Expand All @@ -2567,7 +2579,7 @@ pub(crate) fn define(
r#"
Bitwise or with immediate.

Same as `bor`, but one operand is an immediate constant.
Same as `bor`, but one operand is a sign extended 64 bit immediate constant.

Polymorphic over all scalar integer types, but does not support vector
types.
Expand All @@ -2584,7 +2596,7 @@ pub(crate) fn define(
r#"
Bitwise xor with immediate.

Same as `bxor`, but one operand is an immediate constant.
Same as `bxor`, but one operand is a sign extended 64 bit immediate constant.

Polymorphic over all scalar integer types, but does not support vector
types.
Expand Down Expand Up @@ -2633,6 +2645,8 @@ pub(crate) fn define(
"rotl_imm",
r#"
Rotate left by immediate.

Same as `rotl`, but one operand is a sign extended 64 bit immediate constant.
"#,
&formats.binary_imm64,
)
Expand All @@ -2645,6 +2659,8 @@ pub(crate) fn define(
"rotr_imm",
r#"
Rotate right by immediate.

Same as `rotr`, but one operand is a sign extended 64 bit immediate constant.
"#,
&formats.binary_imm64,
)
Expand Down
184 changes: 40 additions & 144 deletions cranelift/codegen/src/legalizer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
use crate::cursor::{Cursor, FuncCursor};
use crate::flowgraph::ControlFlowGraph;
use crate::ir::types::I32;
use crate::ir::types::{I128, I64};
use crate::ir::{self, InstBuilder, InstructionData, MemFlags};
use crate::isa::TargetIsa;

Expand Down Expand Up @@ -157,160 +157,56 @@ pub fn simple_legalize(func: &mut ir::Function, cfg: &mut ControlFlowGraph, isa:
offset,
} => expand_table_addr(isa, inst, &mut pos.func, table, arg, offset),

// bitops
InstructionData::BinaryImm64 {
opcode: ir::Opcode::BandImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).band(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::BorImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).bor(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::BxorImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).bxor(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::IaddImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).iadd(arg, imm);
}

// bitshifting
InstructionData::BinaryImm64 {
opcode: ir::Opcode::IshlImm,
arg,
imm,
} => {
let imm = pos.ins().iconst(I32, imm);
pos.func.dfg.replace(inst).ishl(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::RotlImm,
arg,
imm,
} => {
let imm = pos.ins().iconst(I32, imm);
pos.func.dfg.replace(inst).rotl(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::RotrImm,
arg,
imm,
} => {
let imm = pos.ins().iconst(I32, imm);
pos.func.dfg.replace(inst).rotr(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::SshrImm,
arg,
imm,
} => {
let imm = pos.ins().iconst(I32, imm);
pos.func.dfg.replace(inst).sshr(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::UshrImm,
arg,
imm,
} => {
let imm = pos.ins().iconst(I32, imm);
pos.func.dfg.replace(inst).ushr(arg, imm);
}

// math
InstructionData::BinaryImm64 {
opcode: ir::Opcode::IrsubImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).isub(imm, arg); // note: arg order reversed
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::ImulImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).imul(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::SdivImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).sdiv(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::SremImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).srem(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::UdivImm,
arg,
imm,
} => {
InstructionData::BinaryImm64 { opcode, arg, imm } => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).udiv(arg, imm);
}
InstructionData::BinaryImm64 {
opcode: ir::Opcode::UremImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).urem(arg, imm);
let imm = if ty == I128 {
let imm = pos.ins().iconst(I64, imm);
pos.ins().sextend(I128, imm)
} else {
pos.ins().iconst(ty.lane_type(), imm)
};

let replace = pos.func.dfg.replace(inst);
match opcode {
// bitops
ir::Opcode::BandImm => replace.band(arg, imm),
ir::Opcode::BorImm => replace.bor(arg, imm),
ir::Opcode::BxorImm => replace.bxor(arg, imm),
// bitshifting
ir::Opcode::IshlImm => replace.ishl(arg, imm),
ir::Opcode::RotlImm => replace.rotl(arg, imm),
ir::Opcode::RotrImm => replace.rotr(arg, imm),
ir::Opcode::SshrImm => replace.sshr(arg, imm),
ir::Opcode::UshrImm => replace.ushr(arg, imm),
// math
ir::Opcode::IaddImm => replace.iadd(arg, imm),
ir::Opcode::IrsubImm => replace.isub(imm, arg), // note: arg order reversed
ir::Opcode::ImulImm => replace.imul(arg, imm),
ir::Opcode::SdivImm => replace.sdiv(arg, imm),
ir::Opcode::SremImm => replace.srem(arg, imm),
ir::Opcode::UdivImm => replace.udiv(arg, imm),
ir::Opcode::UremImm => replace.urem(arg, imm),
// comparisons
ir::Opcode::IfcmpImm => replace.ifcmp(arg, imm),
_ => unimplemented!(),
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 it's a behavioral change if we panic here. The outer match just skips the instruction if it doesn't have an expansion for it. Maybe that's why the previous implementation duplicated so much code? I think @cfallin probably needs to weigh in on how this case should be handled.

Gotta say, though, I really like how much shorter your version is. I hope we can preserve that clarity even if this bit has to change.

It might help to introduce a function that's something like this (but I haven't tested this code, let alone compiled it):

fn imm_const(pos: &mut FuncCursor, arg: Value, imm: Imm64) -> Value {
    let ty = pos.func.dfg.value_type(arg);
    let imm = pos.ins().iconst(ty, imm);
    if ty == I128 {
        let imm = pos.ins().iconst(I64, imm);
        pos.ins().sextend(I128, imm)
    } else {
        pos.ins().iconst(ty.lane_type(), imm)
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we don't want to hit an unimplemented!() here; I think we just want an empty match-arm body (_ => {}) instead. That way we still construct the imm but we just don't do anything with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't an empty match-arm body there cause simple_legalize to loop, trying to legalize the same instruction repeatedly?

Copy link
Member

Choose a reason for hiding this comment

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

The toplevel loop is a while let Some(inst) = pos.next_inst(), so it's stepping through insts with no action taken in the loop body. Though actually the more precise thing to do is to replicate the fallback _ => { ... } below, which sets prev_pos first then continues, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

The statement after the match is pos.set_position(prev_pos); and the intent appears to be to re-examine the result of every legalization. So it looks to me like termination of this function relies on every match arm replacing the instruction that it originally matched on. But yes, copying the prev_pos assignment from the fallback case should work, I think.

};
}

// comparisons
InstructionData::BinaryImm64 {
opcode: ir::Opcode::IfcmpImm,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
pos.func.dfg.replace(inst).ifcmp(arg, imm);
}
InstructionData::IntCompareImm {
opcode: ir::Opcode::IcmpImm,
cond,
arg,
imm,
} => {
let ty = pos.func.dfg.value_type(arg);
let imm = pos.ins().iconst(ty, imm);
let imm = if ty == I128 {
let imm = pos.ins().iconst(I64, imm);
pos.ins().sextend(I128, imm)
} else {
pos.ins().iconst(ty.lane_type(), imm)
};

pos.func.dfg.replace(inst).icmp(cond, arg, imm);
}

Expand Down
10 changes: 10 additions & 0 deletions cranelift/filetests/filetests/runtests/i128-arithmetic.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,13 @@ block0(v0: i128,v1: i128):
; run: %mul_i128(13, 0x01010101_01010101_01010101_01010101) == 0x0D0D0D0D_0D0D0D0D_0D0D0D0D_0D0D0D0D
; run: %mul_i128(0x00000000_01234567_89ABCDEF_00000000, 0x00000000_FEDCBA98_76543210_00000000) == 0x2236D88F_E5618CF0_00000000_00000000
; run: %mul_i128(0xC0FFEEEE_C0FFEEEE_C0FFEEEE_C0FFEEEE, 0xDECAFFFF_DECAFFFF_DECAFFFF_DECAFFFF) == 0x5ECD38B5_9D1C2B7E_DB6B1E48_19BA1112


; Tests that imm's are sign extended on i128's
; See: https://github.com/bytecodealliance/wasmtime/issues/4568
function %iadd_imm_neg(i128) -> i128 {
block0(v0: i128):
v1 = iadd_imm.i128 v0, -1
return v1
}
; run: %iadd_imm_neg(1) == 0