Stream: git-cranelift

Topic: cranelift / PR #1385 Add ineg legalization for scalar int...

GitHub (Feb 11 2020 at 10:19):

peterdelevoryas opened PR #1385 from master to master:

• Closes #1383
• Cranelift already has custom legalization for ineg on vector integer types, might as well add it for scalar integer types!
• This PR includes one new test case, it's relatively meager, maybe I could add cases for all the integer type variants? (i8, i16, i32, i64). Not sure if that's too much or not!
• I don't know who could review this, maybe bnjbvr or bjorn3?

I added some comments in the diff that refer to concerns I had

GitHub (Feb 11 2020 at 11:30):

bjorn3 submitted PR Review.

GitHub (Feb 11 2020 at 11:30):

bjorn3 created PR Review Comment:

This will need to be added to widen too to legalize i8 and i16.

GitHub (Feb 11 2020 at 11:30):

bjorn3 created PR Review Comment:

This is necessary to get the arg passes to ineg. Adding an unreachable!() as else would be nice though.

GitHub (Feb 11 2020 at 11:30):

bjorn3 submitted PR Review.

GitHub (Feb 11 2020 at 11:35):

peterdelevoryas submitted PR Review.

GitHub (Feb 11 2020 at 11:35):

peterdelevoryas created PR Review Comment:

Ohhhh ok, I was trying to find widen and couldn't find it, let me check again

GitHub (Feb 11 2020 at 11:40):

peterdelevoryas submitted PR Review.

GitHub (Feb 11 2020 at 11:40):

peterdelevoryas created PR Review Comment:

ohhh I get it now, ok I'll make a widen group and add the custom legalization there just for x86, now in the shared-common one, since all this ineg stuff is in x86 right now.

GitHub (Feb 11 2020 at 11:40):

peterdelevoryas edited PR Review Comment.

GitHub (Feb 11 2020 at 18:30):

abrown submitted PR Review.

GitHub (Feb 11 2020 at 18:30):

abrown created PR Review Comment:

I think so; I see that type of thing up above. Something like the following would be good to get both the type and the legalization that failed: panic!("Can't convert ineg of {}", valuetype).

GitHub (Feb 11 2020 at 20:09):

peterdelevoryas submitted PR Review.

GitHub (Feb 11 2020 at 20:09):

peterdelevoryas created PR Review Comment:

Hey, I tried adding this to the widen TransformGroup, but that didn't seem to work. I saw a comment in another file about ordering being significant: maybe I need to add the widen group legalizations before or after the narrow/expand additions?

GitHub (Feb 11 2020 at 21:14):

peterdelevoryas updated PR #1385 from master to master:

• Closes #1383
• Cranelift already has custom legalization for ineg on vector integer types, might as well add it for scalar integer types!
• This PR includes one new test case, it's relatively meager, maybe I could add cases for all the integer type variants? (i8, i16, i32, i64). Not sure if that's too much or not!
• I don't know who could review this, maybe bnjbvr or bjorn3?

I added some comments in the diff that refer to concerns I had

GitHub (Feb 11 2020 at 21:18):

bjorn3 submitted PR Review.

GitHub (Feb 11 2020 at 21:18):

bjorn3 created PR Review Comment:

I think using x86_widen instead of widen at https://github.com/bytecodealliance/cranelift/blob/23e9bdb2d99fb8f554793cb87f1578cc543c355f/cranelift-codegen/meta/src/isa/x86/mod.rs#L36 should fix it.

GitHub (Feb 11 2020 at 22:57):

abrown assigned PR #1385.

GitHub (Feb 12 2020 at 01:16):

peterdelevoryas submitted PR Review.

GitHub (Feb 12 2020 at 01:16):

peterdelevoryas created PR Review Comment:

I thought this would be sufficient to get the transformation to apply to i8/i16, but it doesn't seem to be working? I'm getting stuck debugging this because I can't seem to find the place where we apply the transformations. Maybe this is in the generated Rust file?

GitHub (Feb 12 2020 at 01:22):

peterdelevoryas created PR Review Comment:

nvm, I think I see the spot where x86_narrow/etc gets tied to the x86 legalization, maybe I can fix this

GitHub (Feb 12 2020 at 01:22):

peterdelevoryas submitted PR Review.

GitHub (Feb 12 2020 at 03:37):

peterdelevoryas submitted PR Review.

GitHub (Feb 12 2020 at 03:37):

peterdelevoryas created PR Review Comment:

That did fix it! And I added a test for i16 and that worked too. So, i8, i16, and i32 work. But, i64 doesn't: I think these changes made sure convert_ineg is added to widen, narrow, and expand, is there something missing still?

---- filetests stdout ----
FAIL filetests/isa/x86/legalize-custom.clif: legalizer(%ineg_legalized_i):
filecheck failed:
#0 regex: V=v\d+
#1 regex: BB=block\d+
#2 check: v2 = iconst.i32 1
#3 check: v3 = iconst.i32 0
#4 check: v0 = iconcat v2, v3
#5 nextln: v1 = isub v2, v0
> function %ineg_legalized_i() fast {
>                                 block0:
> [Op1pu_id#b8]                       v2 = iconst.i32 1
                                      ^~~~~~~~~~~~~~~~~
Matched #2: \bv2 = iconst\.i32 1\b
> [Op1pu_id#b8]                       v3 = iconst.i32 0
                                      ^~~~~~~~~~~~~~~~~
Matched #3: \bv3 = iconst\.i32 0\b
> [-]                                 v0 = iconcat v2, v3
                                      ^~~~~~~~~~~~~~~~~~~
Matched #4: \bv0 = iconcat v2, v3\b
> [-]                                 v1 = ineg v0
Missed #5: \bv1 = isub v2, v0\b
> [Op1ret#c3]                         return
> }

GitHub (Feb 12 2020 at 03:39):

peterdelevoryas updated PR #1385 (assigned to abrown) from master to master:

• Closes #1383
• Cranelift already has custom legalization for ineg on vector integer types, might as well add it for scalar integer types!
• This PR includes one new test case, it's relatively meager, maybe I could add cases for all the integer type variants? (i8, i16, i32, i64). Not sure if that's too much or not!
• I don't know who could review this, maybe bnjbvr or bjorn3?

I added some comments in the diff that refer to concerns I had

GitHub (Feb 12 2020 at 03:40):

peterdelevoryas updated PR #1385 (assigned to abrown) from master to master:

• Closes #1383
• Cranelift already has custom legalization for ineg on vector integer types, might as well add it for scalar integer types!
• This PR includes one new test case, it's relatively meager, maybe I could add cases for all the integer type variants? (i8, i16, i32, i64). Not sure if that's too much or not!
• I don't know who could review this, maybe bnjbvr or bjorn3?

I added some comments in the diff that refer to concerns I had

GitHub (Feb 12 2020 at 03:52):

abrown submitted PR Review.

GitHub (Feb 12 2020 at 03:52):

abrown created PR Review Comment:

"Legalize instructions by widening"?

GitHub (Feb 12 2020 at 03:58):

abrown submitted PR Review.

GitHub (Feb 12 2020 at 03:58):

abrown created PR Review Comment:

Not sure I get why v2 and v3 have different types initially: shouldn't they both start out as i8s and be extended? And we have to extend and then reduce because we don't have an encoding for isub.i8? (@peterdelevoryas, no need to answer; this is more for @bjorn3 as he is more familiar with this than I am)

GitHub (Feb 12 2020 at 03:59):

abrown created PR Review Comment:

@bjorn3, I would not expect this iconcat; can't we directly encode an iconst.i64?

GitHub (Feb 12 2020 at 03:59):

abrown submitted PR Review.

GitHub (Feb 12 2020 at 06:08):

peterdelevoryas submitted PR Review.

GitHub (Feb 12 2020 at 06:08):

peterdelevoryas created PR Review Comment:

Ah woops thanks for catching that!

GitHub (Feb 12 2020 at 06:57):

bjorn3 submitted PR Review.

GitHub (Feb 12 2020 at 06:57):

bjorn3 created PR Review Comment:

This file has target i686 before target x86_64. How is that handled? Does it test both or only the first?

GitHub (Feb 12 2020 at 07:01):

bjorn3 submitted PR Review.

GitHub (Feb 12 2020 at 07:01):

bjorn3 created PR Review Comment:

iconst.i8 is legalized as iconst.i32 + ireduce.i8. isub.i8 is legalized as uextend.i32 + isub.i32 + ireduce.i8.

GitHub (Feb 12 2020 at 17:37):

abrown submitted PR Review.

GitHub (Feb 12 2020 at 17:37):

abrown created PR Review Comment:

Learn something new every day: apparently parsing tests accumulates the targets and test_tuples will expand the different targets (in this case since the legalizer needs_isa we should have a test tuple for each target from here).

But then this doesn't make sense: how could these checks pass when target x86_64 is set? Is that the build failure? No, it looks like the current build failure happens because ineg isn't getting legalized at all for i64?

GitHub (Feb 12 2020 at 19:28):

peterdelevoryas submitted PR Review.

GitHub (Feb 12 2020 at 19:28):

peterdelevoryas created PR Review Comment:

Yeah the build failure is happening because ineg still isn't getting legalized for i64, I still haven't figured out why that is. This must be because I need to extend another transform group right? That was the reason for i8 and i16 not getting legalized initially (I think)

GitHub (Feb 12 2020 at 19:38):

bjorn3 submitted PR Review.

GitHub (Feb 12 2020 at 19:38):

bjorn3 created PR Review Comment:

There is also the narrow group for ints > pointer size.

GitHub (Feb 14 2020 at 16:10):

peterdelevoryas updated PR #1385 (assigned to abrown) from master to master:

• Closes #1383
• Cranelift already has custom legalization for ineg on vector integer types, might as well add it for scalar integer types!
• This PR includes one new test case, it's relatively meager, maybe I could add cases for all the integer type variants? (i8, i16, i32, i64). Not sure if that's too much or not!
• I don't know who could review this, maybe bnjbvr or bjorn3?

I added some comments in the diff that refer to concerns I had

GitHub (Feb 14 2020 at 16:15):

peterdelevoryas updated PR #1385 (assigned to abrown) from master to master:

• Closes #1383
• Cranelift already has custom legalization for ineg on vector integer types, might as well add it for scalar integer types!
• This PR includes one new test case, it's relatively meager, maybe I could add cases for all the integer type variants? (i8, i16, i32, i64). Not sure if that's too much or not!
• I don't know who could review this, maybe bnjbvr or bjorn3?

I added some comments in the diff that refer to concerns I had

GitHub (Feb 14 2020 at 16:16):

peterdelevoryas submitted PR Review.

GitHub (Feb 14 2020 at 16:16):

peterdelevoryas created PR Review Comment:

Ok, this should be fixed now

GitHub (Feb 14 2020 at 16:17):

peterdelevoryas submitted PR Review.

GitHub (Feb 14 2020 at 16:17):

peterdelevoryas created PR Review Comment:

I added this panic message

GitHub (Feb 14 2020 at 16:17):

peterdelevoryas submitted PR Review.

GitHub (Feb 14 2020 at 16:17):

peterdelevoryas created PR Review Comment:

Added an else { unreachable!() } clause

GitHub (Feb 14 2020 at 16:43):

bjorn3 submitted PR Review.

GitHub (Feb 14 2020 at 18:29):

abrown submitted PR Review.

GitHub (Feb 14 2020 at 18:29):

abrown created PR Review Comment:

I want to make sure I understand this: x86_narrow is chained with narrow_flags so this just makes x86_32 consistent with x86_64--i.e. was this necessary to get this patch to work?

GitHub (Feb 14 2020 at 18:30):

abrown submitted PR Review.

GitHub (Feb 14 2020 at 18:30):

abrown created PR Review Comment:

Perhaps we should rename this to x86_widen (here and where used) to be very clear what is going on.

GitHub (Feb 14 2020 at 18:30):

abrown edited PR Review Comment.

GitHub (Feb 14 2020 at 18:31):

peterdelevoryas created PR Review Comment:

That's correct, I think before this, the legalization of ineg.i64 was not being applied on i686. I'll double check really quickly though and show the error here

GitHub (Feb 14 2020 at 18:31):

peterdelevoryas submitted PR Review.

GitHub (Feb 14 2020 at 18:31):

peterdelevoryas submitted PR Review.

GitHub (Feb 14 2020 at 18:31):

peterdelevoryas created PR Review Comment:

Oh yeah I can definitely do that!

GitHub (Feb 14 2020 at 18:35):

abrown submitted PR Review.

GitHub (Feb 14 2020 at 18:35):

abrown created PR Review Comment:

Sounds good, I'll merge this after your push.

GitHub (Feb 14 2020 at 19:53):

peterdelevoryas created PR Review Comment:

running 1 test
test filetests ... FAILED

failures:

---- filetests stdout ----
FAIL filetests/isa/x86/legalize-i64.clif: legalizer(%ineg_legalized_i):
filecheck failed:
#0 regex: V=v\d+
#1 check: v2 = iconst.i32 1
#2 nextln: v3 = iconst.i32 0
#3 nextln: v0 = iconcat v2, v3
#4 nextln: v5 = iconst.i32 0
#5 nextln: v6 = iconst.i32 0
#6 nextln: v4 = iconcat v5, v6
#7 nextln: v7, v8 = isub_ifbout v5, v2
#8 nextln: v9 = isub_ifbin v6, v3, v8
#9 nextln: v1 = iconcat v7, v9
> function %ineg_legalized_i() fast {
>                                 block0:
> [Op1pu_id#b8]                       v2 = iconst.i32 1
                                      ^~~~~~~~~~~~~~~~~
Matched #1: \bv2 = iconst\.i32 1\b
> [Op1pu_id#b8]                       v3 = iconst.i32 0
                                      ^~~~~~~~~~~~~~~~~
Matched #2: \bv3 = iconst\.i32 0\b
> [-]                                 v0 = iconcat v2, v3
                                      ^~~~~~~~~~~~~~~~~~~
Matched #3: \bv0 = iconcat v2, v3\b
> [-]                                 v1 = ineg v0
Missed #4: \bv5 = iconst\.i32 0\b
> [Op1ret#c3]                         return
> }

(with this diff from the current stuff)

diff --git a/cranelift-codegen/meta/src/isa/x86/mod.rs b/cranelift-codegen/meta/src/isa/x86/mod.rs
index 52d03f54..3fbd78b7 100644
--- a/cranelift-codegen/meta/src/isa/x86/mod.rs
+++ b/cranelift-codegen/meta/src/isa/x86/mod.rs
@@ -32,12 +32,13 @@ pub(crate) fn define(shared_defs: &mut SharedDefinitions) -> TargetIsa {
     let mut x86_32 = CpuMode::new("I32");

     let expand_flags = shared_defs.transform_groups.by_name("expand_flags");

+    let narrow_flags = shared_defs.transform_groups.by_name("narrow_flags");
     let widen = shared_defs.transform_groups.by_name("x86_widen");
     let x86_narrow = shared_defs.transform_groups.by_name("x86_narrow");
     let x86_expand = shared_defs.transform_groups.by_name("x86_expand");

     x86_32.legalize_monomorphic(expand_flags);

-    x86_32.legalize_default(x86_narrow);
+    x86_32.legalize_default(narrow_flags);
     x86_32.legalize_type(B1, expand_flags);
     x86_32.legalize_type(I8, widen);
     x86_32.legalize_type(I16, widen);

GitHub (Feb 14 2020 at 19:53):

peterdelevoryas submitted PR Review.

GitHub (Feb 14 2020 at 19:57):

peterdelevoryas updated PR #1385 (assigned to abrown) from master to master:

• Closes #1383
• Cranelift already has custom legalization for ineg on vector integer types, might as well add it for scalar integer types!
• This PR includes one new test case, it's relatively meager, maybe I could add cases for all the integer type variants? (i8, i16, i32, i64). Not sure if that's too much or not!
• I don't know who could review this, maybe bnjbvr or bjorn3?

I added some comments in the diff that refer to concerns I had

GitHub (Feb 14 2020 at 21:15):

abrown submitted PR Review.

GitHub (Feb 14 2020 at 21:16):

abrown merged PR #1385.

Last updated: Dec 23 2024 at 13:07 UTC