wasmtime / PR #8362 x64: Refactor float comparisons and t... · git-wasmtime

Stream: git-wasmtime

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

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:

dst is renamed to src1

src is renamed to src2

The xmm_cmp_rm_r helpers, and callers, swapped their arguments to take Xmm first and XmmMem second.

The x64_ptest instruction followed suit as it was modelled after the same.

Callers of x64_ucomis swapped their arguments to preserve the operand orders.

The Inst::xmm_cmp_rm_r helper swapped operand order and additionally callers were updated.

The VCode rendering of XmmCmpRmR swapped order of its operands, explaining changes in rendering of filetests (although machine code is not changing here).

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.

Wasmtime GitHub notifications bot (Apr 13 2024 at 18:29):

alexcrichton requested cfallin for a review on PR #8362.

Wasmtime GitHub notifications bot (Apr 13 2024 at 18:29):

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

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:

saulecabrera: winch

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

Learn more.
</details>

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):

In general the x64 backend matches AT&T argument order and syntax, not Intel order, so "matching Intel manual more closely" is moving in the wrong direction unfortunately (at least with respect to current design!). In #6461 @elliottt tried to transition everything to Intel syntax but it turned out to be a too-large undertaking with unexpected interactions...

Given that, I am in general somewhat suspicious of changes like this because the current order is baked in everywhere and it will possibly leave a wake of subtle bugs to be discovered later.

To be clear I am not saying this PR is not careful and carefully checked, just that the risk is high! And the "double swaps" you mention later only add to my caution...

This doesn't mean we can't do it; it does mean that we need clear reasons to do it though, more than just "matches the manuals", something more along the lines of "would have prevented these bugs" or "makes implementing future work X significantly easier" to counterbalance the "change an underlying assumption baked in everywhere" cost. It's totally possible we could pass that threshold at some point. Could you say more about the context/reasoning/path that led to this?

In any case, we should match the order and idioms of the integer comparison instructions; CmpRmiR is also src/dst; is there a plan to refactor that as well to match?

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:

This comment explicitly mentions the swapped order is confusing.

This rule swaps the conditional but doesn't swap the arguments (since the instruction swaps the arguments)

This rules swap the arguments but don't explain why

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.

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?

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?

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?

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."

Wasmtime GitHub notifications bot (Apr 19 2024 at 00:35):

alexcrichton submitted PR review.

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:

Operands are left-to-right and ordered first. The left-to-right ness is Intel and the ordered first is att.

Results are last, which is att-style

Wasmtime GitHub notifications bot (Apr 19 2024 at 00:36):

alexcrichton updated PR #8362.

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.

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.

Wasmtime GitHub notifications bot (Apr 19 2024 at 02:15):

alexcrichton merged PR #8362.

Last updated: Nov 22 2024 at 16:03 UTC