Skip to content

Commit

Permalink
Simplify inverted conditional jumps
Browse files Browse the repository at this point in the history
Summary:
Currently, inverted conditional jumps incur a taken branch in both the
taken and not-taken cases. Use the negated condition code instead so
we do not take any branches in the not-taken case.

Note that it is possible to further improve the taken case when the
operands are not known to be numbers by using a third set of condition
codes which are like the inverted ones but produce false for NaN. If we
do that, we can do the taken jump before the NaN check, knowing that the
NaN check afterwards will take us to the slow path. However, this will
diverge from the known-number case, since that does not have a NaN check
and so will need to use the form in this diff.

Reviewed By: tmikov

Differential Revision: D65185512

fbshipit-source-id: a7316d576f49a3f01c0687e5f7062c231744432a
  • Loading branch information
neildhar authored and facebook-github-bot committed Nov 6, 2024
1 parent aef3bb5 commit 032f59a
Showing 1 changed file with 12 additions and 12 deletions.
24 changes: 12 additions & 12 deletions lib/VM/JIT/arm64/JitEmitter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4434,16 +4434,14 @@ void Emitter::jCond(
hwLeft = getOrAllocFRInVecD(frLeft, true);
hwRight = getOrAllocFRInVecD(frRight, true);

// Do the comparison before checking for a number. If the result is true, we
// know that these cannot be NaN, and therefore must be numbers.
a.fcmp(hwLeft.a64VecD(), hwRight.a64VecD());
if (!invert) {

// If the condition is not inverted, then it can only produce true if both
// operands are numbers. Since we use NaN boxing, we know that all non-number
// values will be NaN and therefore produce false. So if the result is true,
// we can take the jump without checking for numbers.
if (!invert)
a.b(condCode, target);
} else {
if (!contLab.isValid())
contLab = a.newLabel();
a.b(condCode, contLab);
}

if (slow) {
// Since HermesValue is NaN-boxed we know that all non-number values will be
Expand All @@ -4454,15 +4452,17 @@ void Emitter::jCond(
a.b_vs(slowPathLab);
}

// If the condition is inverted, it will produce true if one of the operands
// is a NaN, so we can only check it after the slow path check, since it would
// incorrectly be taken for non-numbers.
if (invert)
a.b(target);

if (contLab.isValid())
a.bind(contLab);
a.b(a64::negateCond(condCode), target);

if (!slow)
return;

a.bind(contLab);

// Do this always, since this is the end of the BB.
freeAllFRTempExcept(FR());

Expand Down

0 comments on commit 032f59a

Please sign in to comment.