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

x86_32: legalize {istore,sload,uload}32.i64 #1789

Closed
wants to merge 1 commit into from
Closed

x86_32: legalize {istore,sload,uload}32.i64 #1789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 27 additions & 0 deletions cranelift/codegen/meta/src/shared/legalize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ pub(crate) fn define(insts: &InstructionGroup, imm: &Immediates) -> TransformGro
let isplit = insts.by_name("isplit");
let istore8 = insts.by_name("istore8");
let istore16 = insts.by_name("istore16");
let istore32 = insts.by_name("istore32");
let isub = insts.by_name("isub");
let isub_bin = insts.by_name("isub_bin");
let isub_bout = insts.by_name("isub_bout");
Expand All @@ -108,6 +109,7 @@ pub(crate) fn define(insts: &InstructionGroup, imm: &Immediates) -> TransformGro
let sdiv_imm = insts.by_name("sdiv_imm");
let select = insts.by_name("select");
let sextend = insts.by_name("sextend");
let sload32 = insts.by_name("sload32");
let sshr = insts.by_name("sshr");
let sshr_imm = insts.by_name("sshr_imm");
let srem = insts.by_name("srem");
Expand All @@ -118,6 +120,7 @@ pub(crate) fn define(insts: &InstructionGroup, imm: &Immediates) -> TransformGro
let uextend = insts.by_name("uextend");
let uload8 = insts.by_name("uload8");
let uload16 = insts.by_name("uload16");
let uload32 = insts.by_name("uload32");
let umulhi = insts.by_name("umulhi");
let ushr = insts.by_name("ushr");
let ushr_imm = insts.by_name("ushr_imm");
Expand Down Expand Up @@ -213,6 +216,30 @@ pub(crate) fn define(insts: &InstructionGroup, imm: &Immediates) -> TransformGro
// embedded as part of arguments), so use a custom legalization for now.
narrow.custom_legalize(iconst, "narrow_iconst");

narrow.legalize(
def!(istore32.I64(flags, a, ptr, offset)),
vec![
def!((al, ah) = isplit(a)),
def!(store.I32(flags, al, ptr, offset)),
],
);
Copy link
Contributor

@abrown abrown Jun 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In this legalization we are trying to store the lower 32 bits of an I64 (the controlling type is for the value to store). It makes sense to me that this would be in narrow.


expand.legalize(
def!(a = sload32.I32(flags, ptr, offset)),
vec![
def!(b = load.I32(flags, ptr, offset)),
def!(a = sextend.I64(b)),
],
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to use an I32 pointer (the controlling type) to load the low 32 bits of a register and then sign-extend to 64 bits (from the instruction definition). In 32-bit x86, what is this operation supposed to be doing? If there are no 64-bit registers available for this sign-extension, is sextend.I64 going to legalize further to something else? And is that actually what you want? E.g. if bit 31 is 0, should another register be zeroed out; or if bit 31 is 1, should another register be set to all 1s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is sextend.I64 going to legalize further to something else?

Yes. In cases that I see (and I get no new legalizer failures with the *32 instructions) sextend.I64 is then processed by isplit, and the top half is usually just thrown away. I have no special insight for why Cranelift emits sload32 instructions though.

Copy link
Contributor

Choose a reason for hiding this comment

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

sload32 instuctions are emitted for a certain WASM instruction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, yeah I see it in code_translator.rs. I think a summary of our back and forth here would be helpful as a comment on these legalizations. The only other remaining issue is: why can't this be in the widen group? @bjorn3, is that clear to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

The ctrl typevar is I32, which matches the native pointer size on x86_32, so it requires the expand group. widen is only for ctrl typevars that are smaller than the native pointer size.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good. Yeah, let's add that nice detail to the comment as well.


expand.legalize(
def!(a = uload32.I32(flags, ptr, offset)),
vec![
def!(b = load.I32(flags, ptr, offset)),
def!(a = uextend.I64(b)),
],
);

Copy link
Contributor

Choose a reason for hiding this comment

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

This uses an I32 pointer (the controlling type) to load the low 32 bits of a register and then just extend (unsigned) to 64 bits. In 32-bit x86, I would think that uextend.I64 should just zero out another register?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct that this could be replaced with just iconst, I kept it symmetric but there's no reason other than that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that is exactly how uextend.i64 is legalized on i686.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

for &(ty, ty_half) in &[(I128, I64), (I64, I32)] {
let inst = uextend.bind(ty).bind(ty_half);
narrow.legalize(
Expand Down
34 changes: 34 additions & 0 deletions cranelift/filetests/filetests/isa/x86/legalize-x86_32-loadstore32.clif
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
; Test the legalization of load32/store32.i64 instructions on x86_32.

test compile
target i686

function u0:0(i32, i32, i32) {
block0(v0: i32, v1: i32, v2: i32):
v3 = iconcat v1, v2
; check: v4 = fill v1
; nextln: v5 = fill v0
; nextln: store v4, v5
istore32.i64 v3, v0
return
}

function u0:1(i32) -> i32 {
block0(v0: i32):
; check: v7 = fill v0
; nextln: v4 = load.i32 v7
v1 = sload32.i32 v0
v2, v3 = isplit v1
; check: return v4
return v2
}

function u0:2(i32) -> i32 {
block0(v0: i32):
; check: v6 = fill v0
; nextln: v4 = load.i32 v6
v1 = uload32.i32 v0
v2, v3 = isplit v1
; check: return v4
return v2
}