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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added subscribers: Orlando, jmorse.
jmorse added a comment.

I've had a look at all the code changes but stopped at the tests; looking broadly good, I've got a few questions / improvements though.



================
Comment at: llvm/docs/LangRef.rst:26751
+
+The ``llvm.fake.use`` intrinsic doesn't perform any operation. It takes a single
+value as an operand and is treated as a use of that operand, to force the
----------------
Could we say "is a no-op" or something to even more explicitly express that there's no visible effect of this intrinsic?


================
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.
----------------
Obvious question: why not just make it single-argument then, or are all intrinsics variadic?


================
Comment at: llvm/docs/LangRef.rst:26768
+
+This intrinsic does nothing, but optimizers must count it as a use of its single
+operand and should try to preserve the intrinsic and its position in the
----------------



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


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1104-1105
 
+  // If this is a reload of a spilled register that only feeds into a FAKE_USE
+  // instruction and is not otherwise used, we skip emitting it.
+  if (isLoadFeedingIntoFakeUse(MI))
----------------
IMO: we should absolutely ram it home into the reader that this is not an observable change to the generated code and explain why we remove it. i.e., "As it's unused, removing the load has no effect. The loaded value has been kept alive for debugging by the fake use, but it will be available on the stack, it isn't necessary to put it in a register too".


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1763-1764
       default:
+        // If this is a reload of a spilled register that only feeds into a
+        // FAKE_USE instruction and is not otherwise used, we skip emitting it.
+        if (isLoadFeedingIntoFakeUse(MI))
----------------
(See comment on above hunk)


================
Comment at: llvm/lib/CodeGen/CodeGenPrepare.cpp:2535-2536
+  auto isFakeUse = [&FakeUses](const Instruction *Inst) {
+    if (auto *II = dyn_cast<IntrinsicInst>(Inst)) {
+      if (II->getIntrinsicID() == Intrinsic::fake_use) {
+        // Record the instruction so it can be preserved when the exit block is
----------------
Can we use C++17 `if (decl; cond)` statement's to make this a single lexical scope?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:3027
       llvm_unreachable("Cannot invoke this intrinsic");
+    case Intrinsic::fake_use:
     case Intrinsic::donothing:
----------------
Can we just do without this addition, and have a hard-error (the `llvm_unreachable`) for anyone trying to `invoke` this intrinsic? For this kind of thing, I believe in "fail fast, fail hard".


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:719
+
+  std::vector<Instruction *> FakeUses;
+  // Record the fake uses we found so we can move them to the front of the
----------------
SmallVector to avoid the initial allocation


================
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);
----------------
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?


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


================
Comment at: llvm/lib/Target/X86/X86FloatingPoint.cpp:450
+        else
+            MI.removeOperand(0);
+      }
----------------
clang-format on the indentation


================
Comment at: llvm/lib/Transforms/Scalar/SROA.cpp:3622
 
   struct LoadOpSplitter : public OpSplitter<LoadOpSplitter> {
     AAMDNodes AATags;
----------------
This SROA stuff looks fine to me, but as it's @Orlando 's favourite pass, he might want a closer look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157616