[PATCH] D157616: [ExtendLifetimes][2/4] Implement fake.use intrinsic in LLVM to extend the lifetime of operands

Tue Sep 12 04:25:10 PDT 2023

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

LGTM, with the rider that I'm not sure what the policy is on putting the python test in there. It's fundamentally a debug-info quality test, which is slightly different to the functional tests that we have elsewhere. The failure mode would be where some optimisation decision changes, we get a non-perfect but acceptable debug-info output, and an optimisation writer is inconvenienced by a test that "can't be correctly updated", as it were.

IMO, we should just put a comment in the test file using the script explaining the purpose of the test (ensure that we get 100% coverage when lifetimes are extended) and what to do if the optimisation decisions break the test, i.e. ensure there's a block of code that gets optimised but not fully because of fake uses, that achieves full coverage.

================
Comment at: llvm/lib/IR/Instruction.cpp:882-887
   for (const Instruction *I = getPrevNode(); I; I = I->getPrevNode())
-    if (!isa<DbgInfoIntrinsic>(I) && !(SkipPseudoOp && isa<PseudoProbeInst>(I)))
+    if (!isa<DbgInfoIntrinsic>(I) && !(SkipPseudoOp && isa<PseudoProbeInst>(I))
+        && !(isa<IntrinsicInst>(I) &&
+             cast<IntrinsicInst>(I)->getIntrinsicID() == Intrinsic::fake_use))
       return I;
   return nullptr;
----------------
StephenTozer wrote:
> jmorse wrote:
> > IIiiiiinteresting -- I'm slightly twitchy about this because this behaviour isn't symmetric with `getNextNonDebugInstruction`, and fake uses are supposed to be observable no-op uses so that we really _do_ suppress optimisations in favour of debuggability. Are you aware of any functional reasons why this change is needed?
> > 
> > (I'm also trying to suppress this loop for performance reasons in the DDD/RemoveDIs stack of patches).
> This should be symmetric with `getNextNonDebugInstruction`, so that does need changing; broad principle here is that we only really want the fake uses to be "uses", while for all operations (I think) where we want to find the next/previous instruction, we would want to ignore them. They need a position to mark the end of the extended lifetime, and they need to be treated as a use, but there's no value for example in having them block a peephole optimization.
Cool -- just to give the full context, we can reclaim about 0.3% of compile time in _non_ -g builds if we don't have all the branching here when moving forwards one instruction. That's 0.3% I'd like to spend on RemoveDIs ideally.  I think at some point we'll need to measure the performance impact of those peephole optimisations being blocked and see whether it's worth the tradeoff.

================
Comment at: llvm/test/DebugInfo/X86/fake-use.ll:7
+; RUN: %llc_dwarf -O2 -filetype=obj -dwarf-linkage-names=Abstract < %s | llvm-dwarfdump -v - -o %t
+; RUN: %python %p/Inputs/check-fake-use.py %t
+
----------------
Run line at top pls

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157616/new/

https://reviews.llvm.org/D157616