Stream: git-wasmtime

Topic: wasmtime / PR #8362 x64: Refactor float comparisons and t...


view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2024 at 18:29):

alexcrichton opened PR #8362 from alexcrichton:x64-refactor-float to bytecodealliance:main:

Currently the XmmCmpRmR instruction variant has a dst and a src field. The instruction doesn't actually write to dst, however, and the constructor of xmm_cmp_rm_r takes the src first followed by the dst. This is inconsistent with most other xmm-related instructions where the "src1" comes first and the "src2", which is a memory operand, comes second. This memory-operand-second pattern also matches the Intel manuals more closely.

This commit refactors the XmmCmpRmR instruction variant with the following changes:

The changes were then additionally propagated to Winch as well. In Winch the src/dst naming was inherited so it was renamed to src1 and src2 which swapped operands as well. In the case of Winch there was additionally an accident in float_cmp_op where values were popped in reverse order. This swapping-of-swapping all worked out prior, but to get all the names to align correctly I've swapped this to be more consistent. Sorry there's a lot of swaps-of-swaps here but the basic idea is that the low-level instruction constructor swapped arguments so to preserve the same (correct) output today something else needed to be swapped. In Winch's case it wasn't the immediate caller of the instruction constructor since that method looked correct, but it was instead a higher-level float_cmp_op which then called a helper which then called the low-level constructor which had operands swapped.

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2024 at 18:29):

alexcrichton requested cfallin for a review on PR #8362.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2024 at 18:29):

alexcrichton requested wasmtime-compiler-reviewers for a review on PR #8362.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2024 at 18:44):

github-actions[bot] commented on PR #8362:

Subscribe to Label Action

cc @saulecabrera

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "winch"

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Apr 13 2024 at 18:58):

cfallin commented on PR #8362:

A few quick thoughts (and thorough review later, work trip this week for some added latency but I should be able to review):

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

alexcrichton commented on PR #8362:

Ah ok, given what you're saying I should nix that bit of the motivation. Despite that I still think that this PR should land. Currently it appears that the backend does not consistently use either ordering operands (intel or att). Almost all simd-related instructions are Xmm XmmMem which is the intel ordering, and I see you're right that x64_cmp is GprMemImm Gpr. Personally I find that confusing because the flags are set comparing src2 to src1, not the other way around, so understanding as-is is not easy. I did not have plans to change CmpRmiR but I can try to find time after this to make it more consistent.

For links to why I think the current state of affairs is confusing and shouldn't be used as rationale to keep things the way they are:

I also wouldn't say that the current approach today is exactly bullet-proof. For example the existing Winch method, which inherits the naming from the Cranelift instruction is actually comparing dst <> src which ended up hiding the issue in codegen where the operands to the instruction were accidentally swapped.

I've tried to audit all usages of x64_ucomis (there aren't many), and it's definitely something I'm trying to cover as I realize these sorts of swap issues can be subtle.

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

cfallin submitted PR review:

OK, I agree a refactor seems reasonable here. Thanks for going through the history and finding examples of this biting us / being confusing!

A note below on the pretty-printing -- that has slightly different tradeoffs IMHO (open to discussion).

Finally: would you mind adding a sentence or two somewhere, maybe at the top of inst.isle, noting that we mostly follow AT&T arg order in the core instruction set, Intel order in SIMD Instructions, and intend to harmonize this at some point?

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

cfallin submitted PR review:

OK, I agree a refactor seems reasonable here. Thanks for going through the history and finding examples of this biting us / being confusing!

A note below on the pretty-printing -- that has slightly different tradeoffs IMHO (open to discussion).

Finally: would you mind adding a sentence or two somewhere, maybe at the top of inst.isle, noting that we mostly follow AT&T arg order in the core instruction set, Intel order in SIMD Instructions, and intend to harmonize this at some point?

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

cfallin created PR review comment:

The changes to the disassembly order are a little confusing to me here since we're still retaining AT&T-style percent-register names (and pretty-printing this alongside other AT&T-syntax insts). At one point the text was even assemblable with GNU as (though that's almost certainly not the case anymore due to pseudoinsts!).

So even though we're swapping the args in the Rust enum and the ISLE-level helper, I think we should probably keep unchanged pretty-printing output here?

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

cfallin created PR review comment:

Can we add to the comment above a note about arg order here, e.g. "compares with src1 as LHS and src2 as RHS, so that e.g. a less-than comparison tests src1 < src2."

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2024 at 00:35):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2024 at 00:35):

alexcrichton created PR review comment:

Sure yeah, I'll note though that in my opinion there's not a ton of consistency right now. Most instructions are printed as a blend of att/intel syntax:

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2024 at 00:36):

alexcrichton updated PR #8362.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2024 at 00:40):

alexcrichton commented on PR #8362:

would you mind adding a sentence or two somewhere, maybe at the top of inst.isle, noting that we mostly follow AT&T arg order in the core instruction set, Intel order in SIMD Instructions, and intend to harmonize this at some point?

After reviewing the instructions I've decided to omit this. By my read it's actually only comparison-related instructions that suffer from this. Everything we have is left-to-right argument order to the x64_* instruction helpers except x64_{test,cmp,cmp_imm} instructions. Given that I don't think a comment's going to do much good since it's probably not going to be read much anyway and most new instructions are going to be copied from existing instructions. I hope to have a follow-up PR to handle the comparison-related instructions.

I also learned that the comparison-related instructions are also confusing because the instruction helpers name the operands src1 and src2 but the fields in the instruction are dst and src.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2024 at 01:37):

alexcrichton commented on PR #8362:

I've opened https://github.com/bytecodealliance/wasmtime/pull/8408 for the general-purpose version of this PR, so I'm going to go ahead and flag this for merge given the approval.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 19 2024 at 02:15):

alexcrichton merged PR #8362.


Last updated: Dec 23 2024 at 12:05 UTC