Better b.cond usage on AArch64 #6305
Conversation
When we're lowering a conditional jump, we previously had a bit of a complicated setup where we could emit a conditional jump to skip over a jump that was the next instruction, and then write out the destination and use a branch register. Now instead we use the b.cond instruction if our offset fits (not common, but not unused either) and if it doesn't we write out an inverse condition to jump past loading the destination and branching directly.
kddnewton
requested review from
maximecb,
XrXr,
tenderlove and
k0kubun
as code owners
yjit/src/backend/arm64/mod.rs
Outdated
| @@ -562,67 +562,45 @@ impl Assembler | |||
|
|
|||
| /// Emit a conditional jump instruction to a specific target. This is | |||
| /// called when lowering any of the conditional jump instructions. | |||
| fn emit_conditional_jump<const CONDITION: u8>(cb: &mut CodeBlock, target: Target) { | |||
| fn emit_conditional_jump<const CONDITION: u8, const INVERSE: u8>(cb: &mut CodeBlock, target: Target) { | |||
There was a problem hiding this comment.
IMO you should implement a negate method for your condition enum. Then you wouldn't have to specify both the condition and its inverse. Seems more succint and less error-prone.
There was a problem hiding this comment.
I pushed a branch up for this. I called the function inverse rather than negate because it felt better to me. But happy to change it.
https://github.com/Shopify/ruby/compare/better-bcond...Shopify:ruby:inverse-condition?expand=1
| @@ -216,7 +217,7 @@ pub fn bcond(cb: &mut CodeBlock, cond: u8, byte_offset: A64Opnd) { | |||
| A64Opnd::Imm(imm) => { | |||
| assert!(bcond_offset_fits_bits(imm), "The immediate operand must be 21 bits or less and be aligned to a 2-bit boundary."); | |||
|
|
|||
| BranchCond::bcond(cond, imm as i32).into() | |||
| BranchCond::bcond(cond, (imm / 4) as i32).into() | |||
There was a problem hiding this comment.
We've discussed before while pairing with Alan and it seems like these divisions and multiplications by 4 keep popping up everywhere. Maybe we should have all the arm jumps take a number of instructions as input instead, since that's ultimately what's being encoded? That could be a separate PR, but something to keep in mind.
Unless you're basing yourself off of a CPU manual or A64 documentation which talks about offsets in bytes?
Prevents the need to pass two params and potentially reduces errors. Co-authored-by: Jimmy Miller <[email protected]>
When we're lowering a conditional jump, we previously had a bit of
a complicated setup where we could emit a conditional jump to skip
over a jump that was the next instruction, and then write out the
destination and use a branch register.
Now instead we use the b.cond instruction if our offset fits (not
common, but not unused either) and if it doesn't we write out an
inverse condition to jump past loading the destination and
branching directly.