x64: Migrate brff and I128 branching instructions to ISLE#4599
x64: Migrate brff and I128 branching instructions to ISLE#4599elliottt merged 4 commits intobytecodealliance:mainfrom
Conversation
d6078dd to
7a9162b
Compare
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
7a9162b to
ddb6807
Compare
400ddb2 to
7388abd
Compare
2d9d8e5 to
8eba11f
Compare
cfallin
left a comment
There was a problem hiding this comment.
This looks great! Happy to see emit_fcmp go away in the handwritten code finally. Just one possible simplification nit below, otherwise good to merge.
| AluRmiROpcode::And8 | AluRmiROpcode::Or8 => true, | ||
| _ => false, | ||
| } | ||
| false |
There was a problem hiding this comment.
Can this go away completely now (i.e. constant-prop the false outward to wherever it's called, if anywhere)?
There was a problem hiding this comment.
Definitely! I was a little unsure if removing Add8 and Or8 was desirable, so I didn't push the refactoring as far as I could have :)
There was a problem hiding this comment.
We likely won't need them for anything else I think -- the actual 8-bit ops use wider opcodes because it's more efficient microarchitecturally (no partial-register stalls) and the upper bits are don't-cares; at least or8 was only needed here because we used SETcc which only sets the lower 8. We can always add them back if we find they're needed!
I changed the lowering of comparisons for
I128to always end withtestinstead ofandorordepending on whether the context was abrzorbrnzinstruction. An effect of this change was that the 8-bit variants of theAndandOralu opcodes were no longer needed. The change that this introduces is visible in thei128.cliftest output update.I've also finished the migration of instructions that require floating point comparisons, and removed all the unused code that stemmed from the rust implementation of
emit_fcmp. This change required adding new constructors to bothSideEffectNoResultsandConsumesFlags, as theAndConditionandOrConditionconstructors ofFcmpCompResultboth lower to two jump instructions. This could be re-implemented as two new pseudo-instructions,JmpCondAndandJmpCondOr, if it seems like this isn't the right approach.The test cases added in
filetests/isa/x64/branches.clifwere generated on the main branch, which helped with debugging thebrz (fcmp ...)lowering.