-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
stage2,aarch64: basic overflow arithmetic support #11574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Great work on the extended test. I will do my best to make them pass for the Wasm backend on Friday. |
Add emitters for `smull`, `umull` and `tst (immediate)` instructions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, apart from some suspicious to32()
and to64()
s
src/arch/aarch64/Emit.zig
Outdated
.smulh => try emit.writeInstruction(Instruction.smulh(rrr.rd.to64(), rrr.rn.to64(), rrr.rm.to64())), | ||
.smull => try emit.writeInstruction(Instruction.smull(rrr.rd.to64(), rrr.rn.to32(), rrr.rm.to32())), | ||
.umulh => try emit.writeInstruction(Instruction.umulh(rrr.rd.to64(), rrr.rn.to64(), rrr.rm.to64())), | ||
.umull => try emit.writeInstruction(Instruction.umull(rrr.rd.to64(), rrr.rn.to32(), rrr.rm.to32())), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The to32()
s and to64()
s should not be here. Mir should always consist of valid assembly instructions with valid register sizes and it should be CodeGen
s job to ensure that. Emit can assume that the register sizes are all valid and shouldn't need to change them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 100%! Lemme fix this!
try self.spillCompareFlagsIfOccupied(); | ||
self.compare_flags_inst = null; | ||
|
||
// TODO this should really be put in a helper similar to `binOpRegister` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed 👍
In collaboration with @joachimschmidt557
Checklist:
add_with_overflow
sub_with_overflow
shl_with_overflow
mul_with_overflow
@joachimschmidt557 and @Luukdegram - seems I have given our three more work as now only
aarch64
backend (excluding LLVM ofc) passes extended@mulWithOverflow
tests ;-)Unblocks the
aarch64
part of @wsengir's #11316