Stream: git-wasmtime

Topic: wasmtime / PR #5888 riscv64: Delete `SelectIf` instruction

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:00):

afonso360 opened PR #5888 from riscv64-remove-selectif to main:

:wave: Hey,

This PR deletes the Inst::SelectIf pseudo instruction from the RISC-V backend. This instruction was only used when lowering the select_spectre_guard clif instruction in a specific case of the input being preceded by an icmp. That being said, the instruction doesn't really do anything different from the regular select.

This PR removes the instruction and defaults to the regular Inst::Select. We get pretty much exactly the same codegen, with the exception that an additional andi is now added into the generated code. That doesn't seem to be to bad, and we can probably remove it with some additional effort if needed.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:04):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:04):

bjorn3 created PR review comment:

select_spectre_guard shouldn't emit any branches. While the old code here is wrong, fixing it will require the split between select and select_spectre_guard again.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:11):

afonso360 created PR review comment:

I don't think we have a way of doing that with the base ISA on RISC-V. Do you know how other compilers handle this?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:11):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:19):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:19):

bjorn3 created PR review comment:

You can use bit masking: https://github.com/RustCrypto/utils/blob/0256fff4d67edb4956f5200b877f41038c4ef7e7/cmov/src/portable.rs#L46-L47

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:26):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:26):

afonso360 created PR review comment:

Right, but I think that can still be speculated on right? My understanding is that we used cmov on x64 and csel on AArch64 because we had architectural guarantees that they wouldn't be speculated on?

I'm up for rewriting this if it does prevent speculation though.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:27):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:27):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:32):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:32):

bjorn3 created PR review comment:

While it is technically possible, I don't think any cpu would see a benefit in pattern matching this pattern and turning it back into a conditional branch that the branch predictor can speculate.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 27 2023 at 15:32):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 02:34):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 02:34):

jameysharp created PR review comment:

What do you two think is the current state of this PR? I'm not sure what conclusions you've come to from this conversation.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 09:42):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 09:42):

bjorn3 created PR review comment:

I guess this PR is fine for now, but I did like to see a proper implementation of select_spectre_guard in the future.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 10:44):

afonso360 created PR review comment:

I have a version of this pretty much ready to go, but got distracted by the fuzzer SIMD stuff! I'll clean it up a little bit and push those commits.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 10:44):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 11:28):

afonso360 updated PR #5888 from riscv64-remove-selectif to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 11:31):

afonso360 updated PR #5888 from riscv64-remove-selectif to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 11:38):

afonso360 updated PR #5888 from riscv64-remove-selectif to main.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 02 2023 at 11:41):

afonso360 has marked PR #5888 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2023 at 13:24):

afonso360 requested elliottt for a review on PR #5888.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2023 at 13:24):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #5888.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 07 2023 at 13:24):

afonso360 updated PR #5888.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 17:15):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 17:15):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 17:15):

elliottt created PR review comment:

Is the reason we can't use lower_bmask here its ultimate use of gen_select_reg? Would it be bad to rework the lowering of SelectReg to take this approach instead?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 17:15):

elliottt created PR review comment:

        ;; non_zero = ((normalized | ((!normalized) + 1)) >> 63) & 1

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 20:40):

afonso360 updated PR #5888.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 20:55):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 20:55):

afonso360 created PR review comment:

I hadn't even considered that this was a bmask operation! And yeah, I think this is a better lowering than what we currently have. However I check what LLVM produces for bmask and It's slightly better, so I'm going to rework bmask to use that and use that here.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 21:33):

afonso360 updated PR #5888.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 21:43):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 21:43):

afonso360 created PR review comment:

I'm much happier with the output of this now, Thanks!

I'm now re-reading your comment, and realizing there was a second part about SelectReg that I missed.

I have somewhat mixed feelings about making Inst::SelectReg do something like this, It would be quite a bit longer (~4 instructions I think?). But it would be really nice since we could move it entirely into ISLE. However I don't think that justifies it, it's a really common operation in the backend.

I'm willing to be persuaded otherwise though!

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 22:03):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 22:03):

elliottt created PR review comment:

My worry with recommending lower_bmask was that it used gen_select_reg internally, which will produce jumps to select the result. Since lower_bmask no longer relies on gen_select_reg, I don't see any reason to rework the SelectReg emit code :)

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 23:18):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 23:18):

elliottt created PR review comment:

Since lower_bmask can already produce an I128 result, I think the following should be equivalent:

        (wide_mask Reg (lower_bmask ty cmp_ty cmp))
        ;; Using the mask above we can select either `x` or `y` by
        ;; performing a bitwise `and` on both sides and then merging them
        ;; together. We know that only the bits of one of the sides will be selected.
        ;; TODO: We can use `andn` here if we have `Zbb`
        (lhs ValueRegs (gen_and arg_ty x wide_mask))
        (rhs ValueRegs (gen_and arg_ty y (gen_bnot arg_ty wide_mask))))

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 23:29):

elliottt edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 10 2023 at 23:31):

elliottt edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 08:49):

afonso360 updated PR #5888.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 16:37):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 16:37):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 16:37):

elliottt created PR review comment:

It would be nice if normalize_cmp_value didn't normalize constants whose value already fit in the target type, but there's no need to fix that on this PR.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 18:15):

afonso360 merged PR #5888.

Last updated: Dec 23 2024 at 13:07 UTC