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

[Stephen Tozer via Phabricator via llvm-commits]

StephenTozer added inline comments.


================
Comment at: llvm/docs/LangRef.rst:26760-26763
+The ``llvm.fake.use`` intrinsic takes one argument, which may be any
+function-local SSA value. Note that the signature is variadic so that the
+intrinsic can take any type of argument, but passing more than one argument will
+result in an error.
----------------
jmorse wrote:
> Obvious question: why not just make it single-argument then, or are all intrinsics variadic?
Not all intrinsics are variadic, but only variadic intrinsics can take a wildcard argument type (so you can have `fake.use(i32)`, `fake.use(f64)`, `fake.use(ptr)` etc).


================
Comment at: llvm/include/llvm/Analysis/PtrUseVisitor.h:289-290
+    // transforming it.
+    // FIXME: revisit this and find another way to deal with the resulting
+    // inconsistencies in SROA.
+    case Intrinsic::fake_use:
----------------
jmorse wrote:
> The word "inconsistencies" is a bit scary, would you know whether there are serious repercussions, or is this a pragmatic-ish "Escape all fake-used ptrs to stop them getting optimised out" approach?
> 
> A little explanation of the motivation for anyone unfamiliar with fake_use's stumbling onto this code would help too.
Looking back at this, I'm not sure whether this is still an issue, as it seemed to trigger asserts in situations involving pointer bitcasts, which we don't see any more with opaque pointers; I'll do some further testing, but pull it out of the review for now until I'm confident it's necessary.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:723-727
+  for (BasicBlock::iterator Inst = I; Inst != End; Inst++)
+    if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(Inst))
+      if (II->getIntrinsicID() == Intrinsic::fake_use)
+        if (!opDefdByOrAfter(Inst, I))
+          FakeUses.push_back(&*Inst);
----------------
jmorse wrote:
> This loop is awkwardly quadratic, in that the outer loop iterates from the tail call to End, and opDefdByOrAfter iterates from the tail call to "Inst". Is there any way we can restructure it? Maybe if sdag uses DominatorTree we can use the local-ordering-number cache to do this quicky?
Yeah, this could easily be done with either a SmallSet or `Instruction::comesBefore`; I'm not sure which would have a better performance profile (SmallSet = O(1) but with more overhead, comesBefore = O(n) if order is invalidated but very little overhead after order achieved), but the latter sounds more promising and readable I think.


================
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;
----------------
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.


================
Comment at: llvm/test/CodeGen/X86/fake-use-escape.ll:1
+; RUN: opt -O2 -S %s | FileCheck %s
+;
----------------
jmorse wrote:
> This is widely targeted; is there any further information about what pass provoked this test? Having a whole-O2 test, especially in the "CodeGen" dir, might seem like overkill.
Hm, looks like this test was included by mistake!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157616