wasmtime / PR #3682 x64: port select to ISLE · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #3682 x64: port select to ISLE

Wasmtime GitHub notifications bot (Jan 11 2022 at 22:03):

abrown opened PR #3682 from isle-select to main:

This change ports CLIF's select instruction to ISLE for x64.

Wasmtime GitHub notifications bot (Jan 11 2022 at 22:11):

abrown requested fitzgen for a review on PR #3682.

Wasmtime GitHub notifications bot (Jan 11 2022 at 22:11):

abrown requested cfallin for a review on PR #3682.

Wasmtime GitHub notifications bot (Jan 11 2022 at 22:13):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Jan 11 2022 at 23:39):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Jan 11 2022 at 23:41):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2022 at 23:41):

abrown created PR review comment:

@cfallin, I believe this test is incorrect--both previously and with the current lowering. By first moving %xmm0 to %xmm1 with movaps, the flag-checking jumps really have no effect (they just move almost the same data again). Thoughts?

Wasmtime GitHub notifications bot (Jan 11 2022 at 23:42):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jan 11 2022 at 23:42):

abrown created PR review comment:

Zooming out: not only was the FloatCC::Equal lowering incorrect for FP, but there are no tests (that I can see) that exercise the other variants.

Wasmtime GitHub notifications bot (Jan 11 2022 at 23:43):

abrown edited PR review comment.

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:51):

cfallin created PR review comment:

This overwrites dst from above, I think -- it seems we want the first cmove to write a tmp, and alternative in the second to be that tmp? So then we have the equivalent of cmove(cc1, a, cmove(cc2, b, alternative)).

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:51):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:57):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:57):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:57):

fitzgen created PR review comment:

Could this have a comment describing what it is?

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:57):

fitzgen created PR review comment:

The is_reg prefix makes me think that these will extract from Regs but they actually will extract from Types. Can we rename these to is_{xmm,gpr}_type instead perhaps?

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:57):

fitzgen created PR review comment:

So this is
dst = if cc1 || cc2 { consequent } else { alternative }
right?

Could we maybe add that pseudocode to the comments for these variants? I think that will be very helpful for future readers.

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:57):

fitzgen created PR review comment:

Sounds like we need some runtests for these selects, rather than just compile tests.

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:57):

fitzgen created PR review comment:

;;;; Rules for `select` ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

;; CLIF `select` instructions receive a testable argument (i.e. boolean or

Wasmtime GitHub notifications bot (Jan 12 2022 at 18:57):

fitzgen created PR review comment:

Alternatively, we could have them operate on Regs and keep the current names. Then we would just have to adjust the call sites appropriately, which I think would be possible for all call sites since they also have Regs available.

Wasmtime GitHub notifications bot (Jan 15 2022 at 01:19):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Jan 27 2022 at 23:29):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Jan 27 2022 at 23:43):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Jan 29 2022 at 01:37):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Jan 29 2022 at 01:38):

abrown has marked PR #3682 as ready for review.

Wasmtime GitHub notifications bot (Jan 29 2022 at 01:48):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen created PR review comment:

I think these hard-coded sizes are fine, since we know an i128 is represented with two i64 GPRs.

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen created PR review comment:

Doing the emit here is actually unsafe because nothing is guaranteeing that

we just emitted the flags producer before this

nothing else is emitted after the flags producer and before this flags consumer that clobbers the flags

What we want to do is return two ConsumesFlags here that we can then pass into with_flags_2 along with a FlagsProducer.

ISLE doesn't have tuple/fixed-size-arrays support, so we will have to define an extern type for this.

But! The other rule for cmove_from_values for when it is a single-register value emits only one ConsumesFlags, so I think we actually want an extern type that is either one or two ConsumesFlags. I think we can just use a SmallVec<[ConsumesFlags; 2]>.

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen created PR review comment:

I think that cmove_from_values should take these ValueRegs instead of Values. This lets us reuse it in places where we've already put the values into registers.
(decl cmove_from_values (Type CC ValueRegs ValueRegs) ConsumesFlags)
(rule (cmove_from_vlaue $I128 cc consequent alternative)
      ...)
````
~~~

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen created PR review comment:

These rules look great, thanks!

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen created PR review comment:

Note that we will want a with_flags_* variant that takes the SmallVec<[ConsumesFlags; 2]> as well, and it will probably have to be implemented in external Rust code so it can loop over each ConsumesFlags in the small vec.

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen created PR review comment:

We need to have an extractor that ensure that this is a single-register type, as discussed before:
(rule (cmove_from_values (is_single_register_type ty) cc consequent alternative)

Wasmtime GitHub notifications bot (Jan 31 2022 at 19:16):

fitzgen created PR review comment:

Nitpick: we should clarify how we are combining the multiple flags here:

;; Helper for creating `cmove` instructions with the logical OR of multiple
;; flags. Note that these instructions will always result in more than one
;; emitted x86 instruction.

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:08):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:08):

abrown created PR review comment:

The downside to that is that it bloats the rules in lower.isle; can we do this refactoring in a future PR or issue, e.g., the next time cmove is used?

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:11):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:14):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:14):

abrown created PR review comment:

This sounds like a lot of new infrastructure to build and this PR may already have enough of that. The current lowering rules perform the same unsafe emission as this PR. Can't this be done afterwards?

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:17):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:18):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:18):

abrown created PR review comment:

Ok, I added an extractor that just checks that the type is not I128 in fb334e1--can you take a look at that?

Wasmtime GitHub notifications bot (Feb 01 2022 at 21:19):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Feb 01 2022 at 22:31):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2022 at 22:31):

fitzgen created PR review comment:

Sure, that works for me.

Wasmtime GitHub notifications bot (Feb 01 2022 at 22:49):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 01 2022 at 22:49):

fitzgen created PR review comment:

The current lowering rules for select are written in Rust, so it isn't really comparable. It would be one thing if this emit was near the emit for the flags-producer instruction and the emit for the other flags-consuming instruction, but it isn't. It isn't even clear to me, reading this code, that the instructions will always be emitted in the correct order!

It seems to me that the order of evaluation will be something like

construct FlagsProducer (do not emit it)

call this constructor

emit first cmove

construct FlagsConsumer with the second cmove

call a with_flags variant with the constructed FlagsProducer and FlagsConsumer

emit the producer

emit the consumer

If that is actually how things end up evaluating, then we would get cmove; test/cmp; cmove instead of test/cmp; cmove; cmove.

It looks like we aren't testing select with i128s yet (since you have only ported floating point selects so far). But this is exactly the kind bug I want to avoid with a dedicated with_flags_* variant that takes SmallVec<[ConsumesFlags; 2]>, as described above.

I really think we should fix this issue before landing this PR, or change this PR to not define and use cmove_from_values and instead use plain cmove (which will work for all the single-register values for select that are are currently supported in ISLE).

Wasmtime GitHub notifications bot (Feb 15 2022 at 19:45):

cfallin updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Feb 15 2022 at 19:59):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2022 at 19:59):

abrown created PR review comment:

Ok, @cfallin and I (but mostly @cfallin), worked on this a bit. The unpolished results are in 667935f--@fitzgen, thoughts?

Wasmtime GitHub notifications bot (Feb 15 2022 at 20:57):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2022 at 20:57):

fitzgen created PR review comment:

The extensions to with_flags, ProducesFlags, and ConsumesFlags look good!

Wasmtime GitHub notifications bot (Feb 15 2022 at 20:58):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 15 2022 at 20:58):

fitzgen created PR review comment:

Looks good!

Wasmtime GitHub notifications bot (Feb 18 2022 at 21:12):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Feb 18 2022 at 21:15):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Feb 18 2022 at 21:39):

abrown requested fitzgen for a review on PR #3682.

Wasmtime GitHub notifications bot (Feb 18 2022 at 21:39):

abrown requested cfallin for a review on PR #3682.

Wasmtime GitHub notifications bot (Feb 22 2022 at 18:30):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2022 at 18:45):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 23 2022 at 01:51):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Feb 23 2022 at 04:29):

abrown updated PR #3682 from isle-select to main.

Wasmtime GitHub notifications bot (Feb 23 2022 at 18:03):

abrown merged PR #3682.

Last updated: Dec 23 2024 at 12:05 UTC