Winch: implement rmw and, xor and or for x64#10023
Conversation
github-actions
Bot
added
cranelift
Subscribe to Label ActionDetailsThis 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 |
saulecabrera
requested review from
saulecabrera
and removed request for
a team and
fitzgen
MarinPostma
changed the title
and, and or for x64and, xor and or for x64
MarinPostma
force-pushed
the
winch-x64-rmw-and
branch
3 times, most recently
from
3324880 to
3f8c463
Compare
saulecabrera
left a comment
saulecabrera
left a comment
There was a problem hiding this comment.
Winch pieces look good to me, modulo the panic comment below.
| _ => unreachable!( | ||
| "invalid op for atomic_rmw_seq, should be one of `or`, `and` or `xor`" | ||
| ), |
There was a problem hiding this comment.
Could you return an error here instead of panicking?
There was a problem hiding this comment.
does it make sense to return an error here? we explicitely check that we only match the right variants above, this branch should be legitmately unreachable. No CodeGenError seem to correspond, so I was reluctant to add an error variant for a trivially unreachable statement.
There was a problem hiding this comment.
Yeah you're right, it's probably fine as is. I'm trying to error on the side of caution when it comes to panics, because we've put a lot of effort to make sure that they are minimal, particularly if they affect partial development of features at the visitor/masm layer. Here I got confused by the nested match which led me to think that we were missing an implementation.
| // Instructions (top level): definition | ||
|
|
||
| // `Inst` is defined inside ISLE as `MInst`. We publicly re-export it here. | ||
| pub use super::lower::isle::generated_code::AtomicRmwSeqOp; |
There was a problem hiding this comment.
I don't see an issue with exposing this. However, given that I'm not deeply familiar with the history of x64 backend in Cranelift, I'll kindly ask @cfallin for confirmation.
|
@MarinPostma just a heads up that there are a couple of conflicts that need to be resolved before merging. |
implement rmw or implement atomic rmw xor fix operand sizes implement and, or, xor update rmw or tests update rmw xor tests fmt use ad-hoc conversion for AtomicRmwSeqOp fix test fix rebae quirks
MarinPostma
force-pushed
the
winch-x64-rmw-and
branch
from
3f8c463 to
d1c5659
Compare
|
@saulecabrera I squashed to make conflict resolution more manageable |
2 participants
Implement related atomic rmw for winch x64 backend:
and:i32.atomic.rmw8.and_ui32.atomic.rmw16.and_ui32.atomic.rmw.andi64.atomic.rmw8.and_ui64.atomic.rmw16.and_ui64.atomic.rmw32.and_ui64.atomic.rmw.andor:i32.atomic.rmw8.or_ui32.atomic.rmw16.or_ui32.atomic.rmw.ori64.atomic.rmw8.or_ui64.atomic.rmw16.or_ui64.atomic.rmw32.or_ui64.atomic.rmw.orxor:i32.atomic.rmw8.xor_ui32.atomic.rmw16.xor_ui32.atomic.rmw.xori64.atomic.rmw8.xor_ui64.atomic.rmw16.xor_ui64.atomic.rmw32.xor_ui64.atomic.rmw.xor#9734