alexcrichton opened PR #8362 from alexcrichton:x64-refactor-float
to bytecodealliance:main
:
Currently the
XmmCmpRmR
instruction variant has adst
and asrc
field. The instruction doesn't actually write todst
, however, and the constructor ofxmm_cmp_rm_r
takes thesrc
first followed by thedst
. 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 tosrc1
src
is renamed tosrc2
- The
xmm_cmp_rm_r
helpers, and callers, swapped their arguments to takeXmm
first andXmmMem
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 tosrc1
andsrc2
which swapped operands as well. In the case of Winch there was additionally an accident infloat_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-levelfloat_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:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton requested cfallin for a review on PR #8362.
alexcrichton requested wasmtime-compiler-reviewers for a review on PR #8362.
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>
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?
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 thatx64_cmp
isGprMemImm Gpr
. Personally I find that confusing because the flags are set comparingsrc2
tosrc1
, not the other way around, so understanding as-is is not easy. I did not have plans to changeCmpRmiR
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.
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?
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?
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?
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 andsrc2
as RHS, so that e.g. a less-than comparison testssrc1 < src2
."
alexcrichton submitted PR review.
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
alexcrichton updated PR #8362.
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 exceptx64_{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
andsrc2
but the fields in the instruction aredst
andsrc
.
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.
alexcrichton merged PR #8362.
Last updated: Dec 23 2024 at 12:05 UTC