Skip to content

Commit

Permalink
bindjcc optimization should take into account block coldness
Browse files Browse the repository at this point in the history
Summary:
Suppose you have the following vasm:

`
  B1:
    .....
    jcc B2 B3
  B2:
    bindjmp
  B3 (cold):
    .....
`

the vasm-exits pass will convert this into:

`
  B1:
    .....
    bindjcc
    jmp B3
  B3 (cold):
    .....
`

then, since B1 ends in an unconditional jump to B3, the vasm-jumps pass will
merge B3 into B1. The problem is, B3 is cold, and B1 is not, so B3 won't be
emitted in the cold section.

Avoid this by only performing the above bindjcc optimization if the other
(non-bindjmp target) is not colder than the current block. With this logic, with
the above example, we'll emit a conditional jump into the cold section, and B1
will fallthru into the bindjmp (the common case).

I noticed this while making the reification errors in the function prologue be
marked as unlikely (which causes the above pattern).

Reviewed By: paulbiss

Differential Revision: D13020533

fbshipit-source-id: 0f7a2e15931084a251b36e872b354c49b8ef5405
  • Loading branch information
ricklavoie authored and hhvm-bot committed Nov 12, 2018
1 parent f49d7f3 commit d080125
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 2 deletions.
2 changes: 2 additions & 0 deletions hphp/runtime/vm/jit/irgen-func-prologue.cpp
Expand Up @@ -128,6 +128,7 @@ void emitARHasReifiedGenericsCheck(IRGS& env) {
},
[&] {
if (func->hasReifiedGenerics()) return;
hint(env, Block::Hint::Unlikely);
gen(
env,
RaiseError,
Expand All @@ -137,6 +138,7 @@ void emitARHasReifiedGenericsCheck(IRGS& env) {
[&] {
if (!func->hasReifiedGenerics()) return;
// null out VarEnv before raising error
hint(env, Block::Hint::Unlikely);
gen(env, RaiseError, cns(env, s_reified_generics_not_given.get()));
}
);
Expand Down
9 changes: 7 additions & 2 deletions hphp/runtime/vm/jit/vasm-exits.cpp
Expand Up @@ -123,12 +123,17 @@ void optimizeExits(Vunit& unit) {
code.emplace_back(jmp{next}, irctx);
};

auto const is_colder = [&] (Vlabel succ) {
return unit.blocks[b].area_idx < unit.blocks[succ].area_idx;
};

// Try to replace jcc to normal exit with bindexit followed by jmp,
// as long as the sp adjustment is harmless to hoist (disp==0)
Vptr sp;
if (match_bindjmp(unit, t1, &sp) && sp == sp.base[0]) {
if (match_bindjmp(unit, t1, &sp) && sp == sp.base[0] && !is_colder(t0)) {
fold_exit(ijcc.cc, t1, t0);
} else if (match_bindjmp(unit, t0, &sp) && sp == sp.base[0]) {
} else if (match_bindjmp(unit, t0, &sp) && sp == sp.base[0] &&
!is_colder(t1)) {
fold_exit(ccNegate(ijcc.cc), t0, t1);
}
});
Expand Down

0 comments on commit d080125

Please sign in to comment.