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

whitequark · 2020-05-29T17:46:27Z

Fixes #1747.

github-actions · 2020-05-29T18:00:52Z

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift", "cranelift:meta"

Thus the following users have been cc'd because of the following labels:

bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

whitequark · 2020-05-29T20:03:47Z

Hm, I think I still don't fully understand the machinery here, especially around istore32 and *load8...

cranelift/filetests/filetests/isa/x86/legalize-x86_32-loadstore32-i64.clif

abrown · 2020-06-03T22:37:16Z

I think I'm ok merging this if we add some comments like those in #1747 (comment) about what the controlling type variable is. This isn't really a problem with the PR as much as it is with the types of the instruction definitions being unclear but at least with comments we'll be able to troubleshoot these legalizations more easily. Also, I would have expected to see these legalizations in widen, not expand so if it can't be changed there we should probably explain that as well.

whitequark · 2020-06-03T22:41:23Z

So the problem is that unlike my earlier PRs I actually still don't really understand why it works, and why some of the earlier (incorrect) iterations worked and some of them were broken.

abrown · 2020-06-04T00:14:05Z

Hm, yeah... Cranelift's legalizations are a bit tricky. Just to confirm the intent of all of this, you want to be able to run istore32, sload32, and uload32 on 32-bit x86, right?

whitequark · 2020-06-04T00:16:42Z

Yes. Moreover, I have the exact same problem with the store8/load8 variants of instructions, and I assume also store16/load16 though I haven't encountered those. The problem is that applying the knowledge from this PR to the store8/load8 variants has not been successful as it looks like the meta code requires the cvar to be half the size of the result, whereas they should be independent.

abrown · 2020-06-04T00:17:20Z

cranelift/codegen/meta/src/shared/legalize.rs

+            def!((al, ah) = isplit(a)),
+            def!(store.I32(flags, al, ptr, offset)),
+        ],
+    );


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.

abrown · 2020-06-04T00:25:22Z

cranelift/codegen/meta/src/shared/legalize.rs

+            def!(b = load.I32(flags, ptr, offset)),
+            def!(a = sextend.I64(b)),
+        ],
+    );


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?

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.

sload32 instuctions are emitted for a certain WASM instruction.

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?

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.

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

abrown · 2020-06-04T00:28:38Z

cranelift/codegen/meta/src/shared/legalize.rs

+            def!(a = uextend.I64(b)),
+        ],
+    );
+


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?

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

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

abrown · 2020-06-04T00:31:21Z

My comments are aimed at clarifying what we want these legalizations to do (so feel free to correct me above if I'm not getting the intent right). But it occurs to me that this may be quicker in IM. I'm usually in https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift and can discuss more tomorrow.

Fixes bytecodealliance#1747. Co-authored-by: bjorn3

whitequark · 2020-07-05T08:50:38Z

I think I'm going to close this because I see that the migration to the new backend is underway, and it feels like I'm not going to be able to get x86_32 support to a usable state before it gets removed.

tschneidereit · 2020-07-05T09:13:22Z

@whitequark this might make sense, but just to make sure we didn't accidentally create wrong impressions: the old backend framework won't be removed until sometime next year. We'll have switched the default for the x64 codegen to the new framework well before, after which very little will happen around the old one, but it'll stay in-tree for quite a while longer.

whitequark · 2020-07-05T09:18:33Z

the old backend framework won't be removed until sometime next year

I understand that, but I expect that, once the old backend is no longer heavily tested by the majority of the users, it's going to slowly bit-rot. Since the x86_32 code paths are marginal in first place, it would mean that I'd have to run twice as fast just to remain in the same place, and I don't expect to be able to do that.

whitequark · 2020-07-05T09:23:39Z

Oh and to add to this: I would be generally happy to work on x32 support in the new backend, but given that it's currently written to support x64 alone, it requires an amount of work I can't justify (and experience with x86 that I do not have). If someone else generalizes the x64 backend to support 32 bits too, like the old one, I'd help bring it to completion.

tschneidereit · 2020-07-05T10:09:13Z

Since the x86_32 code paths are marginal in first place, it would mean that I'd have to run twice as fast just to remain in the same place, and I don't expect to be able to do that.

I thought that might be the reason, and makes sense to me, yes.

If someone else generalizes the x64 backend to support 32 bits too, like the old one, I'd help bring it to completion.

Much appreciated! I filed #1980 to look into what support for x86 in the new framework could look like.

whitequark · 2020-07-05T10:12:10Z

I filed #1980 to look into what support for x86 in the new framework could look like.

Thank you! Looking forward to it.

whitequark changed the title ~~[WIP] x86_32: legalize {istore,sload,uload}32.i64~~ x86_32: legalize {istore,sload,uload}32.i64 May 29, 2020

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. labels May 29, 2020

whitequark force-pushed the x86_32-legalize-loadstore32.i64 branch 2 times, most recently from 1d984ba to a96dc5f Compare May 29, 2020 19:46

bjorn3 reviewed May 29, 2020

View reviewed changes

cranelift/filetests/filetests/isa/x86/legalize-x86_32-loadstore32-i64.clif Outdated Show resolved Hide resolved

whitequark force-pushed the x86_32-legalize-loadstore32.i64 branch from a96dc5f to 5b44d6d Compare May 30, 2020 05:59

whitequark marked this pull request as ready for review May 30, 2020 05:59

abrown reviewed Jun 4, 2020

View reviewed changes

alexcrichton changed the base branch from master to main June 25, 2020 18:49

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

ecc4fdc

Fixes bytecodealliance#1747. Co-authored-by: bjorn3

whitequark force-pushed the x86_32-legalize-loadstore32.i64 branch from 5b44d6d to ecc4fdc Compare July 5, 2020 08:50

whitequark closed this Jul 5, 2020

tschneidereit mentioned this pull request Jul 5, 2020

Support x86-32 in new instruction selection framework #1980

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

whitequark commented May 29, 2020 •

edited

Loading

github-actions bot commented May 29, 2020

whitequark commented May 29, 2020

abrown commented Jun 3, 2020

whitequark commented Jun 3, 2020

abrown commented Jun 4, 2020

whitequark commented Jun 4, 2020

abrown Jun 4, 2020 •

edited

Loading

abrown Jun 4, 2020

whitequark Jun 4, 2020

bjorn3 Jun 4, 2020

abrown Jun 4, 2020

bjorn3 Jun 4, 2020

abrown Jun 4, 2020

abrown Jun 4, 2020

whitequark Jun 4, 2020

bjorn3 Jun 4, 2020

abrown Jun 4, 2020

abrown commented Jun 4, 2020 •

edited

Loading

whitequark commented Jul 5, 2020

tschneidereit commented Jul 5, 2020

whitequark commented Jul 5, 2020

whitequark commented Jul 5, 2020

tschneidereit commented Jul 5, 2020

whitequark commented Jul 5, 2020

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

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

Conversation

whitequark commented May 29, 2020 • edited Loading

github-actions bot commented May 29, 2020

Subscribe to Label Action

whitequark commented May 29, 2020

abrown commented Jun 3, 2020

whitequark commented Jun 3, 2020

abrown commented Jun 4, 2020

whitequark commented Jun 4, 2020

abrown Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abrown commented Jun 4, 2020 • edited Loading

whitequark commented Jul 5, 2020

tschneidereit commented Jul 5, 2020

whitequark commented Jul 5, 2020

whitequark commented Jul 5, 2020

tschneidereit commented Jul 5, 2020

whitequark commented Jul 5, 2020

whitequark commented May 29, 2020 •

edited

Loading

abrown Jun 4, 2020 •

edited

Loading

abrown commented Jun 4, 2020 •

edited

Loading