Stream: git-wasmtime

Topic: wasmtime / PR #3954 x64: fix register allocation panic du...

Wasmtime GitHub notifications bot (Mar 21 2022 at 21:17):

abrown opened PR #3954 from isle-issue-3951 to main:

Fuzz testing identified a lowering case for CLIF's icmp in which the
double use of a loaded operand resulted in a register allocation error.
This change manually adds put_in_xmm to avoid load-coalescing these
values and includes a CLIF filetest to trigger this issue. Closes #3951.

I opened #3953 to discuss a way in which this kind of mistake (i.e.,
forgetting to add put_in_* in certain situations) could be avoided.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Mar 21 2022 at 21:18):

abrown requested alexcrichton for a review on PR #3954.

Wasmtime GitHub notifications bot (Mar 21 2022 at 22:03):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 21 2022 at 22:03):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Mar 21 2022 at 22:03):

alexcrichton created PR review comment:

I haven't been following the implicit conversions recently but this feels sort of weird to me where previously when I was working on ISLE I tried to make sure that put_in_xmm was only called once-per-value, but here you're explicitly calling put_in_xmm b above but here it's implicitly being converted with what I presume is also put_in_xmm. I don't personally know the effect of calling put_in_xmm twice and whether it has some sort of cache to repeatedly return the same thing.

Wasmtime GitHub notifications bot (Mar 21 2022 at 22:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 21 2022 at 22:33):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Mar 21 2022 at 22:33):

cfallin created PR review comment:

I would probably write this as (a_reg Xmm (put_in_xmm a)) and then use a_reg afterward. The use of b below feels a bit asymmetrical and confusing otherwise.

Wasmtime GitHub notifications bot (Mar 21 2022 at 22:33):

cfallin created PR review comment:

Likewise here, I'd bind the registers as separate locals then use them here and in the pcmpeq below.

Wasmtime GitHub notifications bot (Mar 21 2022 at 22:33):

cfallin created PR review comment:

There's actually a whole section of our documentation answering this question! Here we added some justification for this decision and a criterion for when it is acceptable: implicit conversions can have side-effects if they are idempotent.

Specifically, put_in_{reg,xmm,gpr} will make a note that the value is required (so when the upward pass reaches the producer, it's actually lowered), and return the vreg already assigned for this SSA value. Calling it multiple times will return the same vreg.

On balance, making put_in_reg implicit makes lowering patterns easier to write, I think; the real issue here is the odd condition that we can't use memory operands in compares. This is (as @abrown notes in sibling issue) something we should fix, for sure.

Wasmtime GitHub notifications bot (Mar 22 2022 at 00:40):

abrown updated PR #3954 from isle-issue-3951 to main.

Wasmtime GitHub notifications bot (Mar 22 2022 at 00:53):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Mar 22 2022 at 00:53):

abrown created PR review comment:

Yeah, good catch; I was doing this half-implicit/half-explicit earlier (and thanks to @alexcrichton also for pointing that out). Fixed in c8d8556.

Wasmtime GitHub notifications bot (Mar 22 2022 at 00:56):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Mar 22 2022 at 00:56):

abrown created PR review comment:

This thread is resolved by the new let statements for xmm_a and xmm_b in c8d8556. I'll leave it unresolved, though, for future readers who may want to understand what @cfallin just explained.

Wasmtime GitHub notifications bot (Mar 22 2022 at 01:46):

abrown merged PR #3954.

Last updated: Oct 23 2024 at 20:03 UTC