This is an archive of the discontinued LLVM Phabricator instance.

[CSSPGO] Unblock optimizations with pseudo probe instrumentation.
ClosedPublic

Authored by hoy on Feb 3 2021, 3:20 PM.

Details

Summary

The IR/MIR pseudo probe intrinsics don't get materialized into real machine instructions and therefore they don't incur runtime cost directly. However, they come with indirect cost by blocking certain optimizations. Some of the blocking are intentional (such as blocking code merge) for better counts quality while the others are accidental. This change unblocks perf-critical optimizations that do not affect counts quality. They include:

  1. IR InstCombine, sinking load operation to shorten lifetimes.
  2. MIR LiveRangeShrink, similar to #1
  3. MIR MachineSinking, similar to #1
  4. MIR TwoAddressInstructionPass, i.e, opeq transform
  5. MIR function argument copy elision
  6. IR stack protection. (though not perf-critical but nice to have).

Diff Detail

Event Timeline

hoy created this revision.Feb 3 2021, 3:20 PM
hoy requested review of this revision.Feb 3 2021, 3:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2021, 3:20 PM
hoy updated this revision to Diff 321246.Feb 3 2021, 3:31 PM

Updating D95982: [CSSPGO] Unblock optimizations with pseudo probe instrumentation.

wmi added a comment.Feb 5 2021, 8:40 PM

Is it possible to add testcases for those cases where optimizations were unblocked by the patch?

hoy added a comment.Feb 6 2021, 9:10 AM
In D95982#2546648, @wmi wrote:

Is it possible to add testcases for those cases where optimizations were unblocked by the patch?

Good point. Let me try add some tests.

hoy updated this revision to Diff 322304.Feb 8 2021, 11:57 PM

Adding test cases for optimizations blocked by pseudo probes.

Test omitted for certain cases where debug instrinsic checks are replaced with debug and pseudo probe checks.

wmi added inline comments.Feb 10 2021, 12:17 AM
llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
805–810

Similar to IR pass, add a MachineInstr::isDebugOrPseudoInstr()?

llvm/lib/Transforms/IPO/FunctionAttrs.cpp
152–157

Make it a little more general?

if (auto *CB = dyn_cast<CallBase>(I))
  if (CB->onlyAccessesInaccessibleMemory())
    continue;
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
595

Make it a little more general in the same way as above.

hoy marked an inline comment as done.Feb 10 2021, 9:09 AM
hoy added inline comments.
llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
805–810

Good suggestion, thanks.

llvm/lib/Transforms/IPO/FunctionAttrs.cpp
152–157

I think we might want to be a bit conservative here, since in theory onlyAccessesInaccessibleMemory still accesses memory. This may lead to

  1. Functions with onlyAccessesInaccessibleMemory calls may not be free to be removed.
  2. Calls with onlyAccessesInaccessibleMemory may not be moved around across each other.

Pseudo probe don't have such effects thus checking against pseudo probes here should have minimal impact. What do you think?

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
595

Sounds good to generalize here since the LoadInst to be sinked doesn't access inaccessible memory.

hoy updated this revision to Diff 322707.Feb 10 2021, 9:09 AM
hoy marked an inline comment as done.

Addressing Wei's feedbacks.

wmi accepted this revision.Feb 10 2021, 11:41 AM

LGTM.

llvm/lib/Transforms/IPO/FunctionAttrs.cpp
152–157

Right. It is better to be conservative here.

This revision is now accepted and ready to land.Feb 10 2021, 11:41 AM
This revision was landed with ongoing or failed builds.Feb 10 2021, 12:43 PM
This revision was automatically updated to reflect the committed changes.