[PATCH] D89739: [LCSSA] Doc for special treatment of PHIs

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis created this revision.
baziotis added reviewers: Meinersbur, fhahn, kbarton, Whitney.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.
baziotis requested review of this revision.

I was taking a look at LCSSA and while most of it is easy, this part confused me a lot. Hopefully I have understood it correctly. We may want to add the same
doc in `isLCSSAForm()`.


https://reviews.llvm.org/D89739

Files:
  llvm/lib/Transforms/Utils/LCSSA.cpp
  llvm/test/Transforms/LCSSA/test.ll


Index: llvm/test/Transforms/LCSSA/test.ll
===================================================================
--- /dev/null
+++ llvm/test/Transforms/LCSSA/test.ll
@@ -0,0 +1,23 @@
+; RUN: opt < %s -passes=lcssa,verify -S | FileCheck %s
+
+define void @foo(i1 %c) {
+entry:
+  br label %header
+
+header:
+  %x = add i32 1, 2
+  br i1 %c, label %latch, label %loop.exit
+
+latch:
+  br i1 %c, label %header, label %intermediate
+
+intermediate:
+; CHECK:    [[X_LCSSA:%.*]] = phi i32 [ %x, %latch ]
+  br label %loop.exit
+
+loop.exit:
+; CHECK:    [[X_NOT_LCSSA:%.*]] = phi i32 [ %x, %header ], [ [[X_LCSSA]], %intermediate ]
+;; %x.not.lcssa acts as a LCSSA PHI for the first use but not the second.
+  %x.not.lcssa = phi i32 [ %x, %header ], [ %x, %intermediate ]
+  ret void
+}
Index: llvm/lib/Transforms/Utils/LCSSA.cpp
===================================================================
--- llvm/lib/Transforms/Utils/LCSSA.cpp
+++ llvm/lib/Transforms/Utils/LCSSA.cpp
@@ -111,6 +111,20 @@
     for (Use &U : I->uses()) {
       Instruction *User = cast<Instruction>(U.getUser());
       BasicBlock *UserBB = User->getParent();
+
+      // A PHI may already act as an LCSSA PHI (note that an LCSSA PHI
+      // is the only instruction that is outside the loop, uses a value
+      // from inside and does _not_ need a rewrite). To check that, we
+      // have to inspect its incoming blocks and not its parent block.
+      // If the incoming block for this use is inside the loop
+      // (which we check below), then this PHI acts as an LCSSA
+      // PHI; only for this use though e.g. this can exist:
+      //
+      // some.exit.block:
+      //   %x.not.lcssa = phi i32 [ %x, %inside.block ], [ %x, %outside.block]
+      //
+      // %x.not.lcssa acts as an LCSSA PHI for the first use of %x
+      // but not the second.
       if (auto *PN = dyn_cast<PHINode>(User))
         UserBB = PN->getIncomingBlock(U);
 
@@ -160,7 +174,8 @@
                                       I->getName() + ".lcssa");
       // Get the debug location from the original instruction.
       PN->setDebugLoc(I->getDebugLoc());
-      // Add inputs from inside the loop for this PHI.
+      // Add inputs from inside the loop for this PHI. This is valid
+      // because `I` dominates `ExitBB`.
       for (BasicBlock *Pred : PredCache.get(ExitBB)) {
         PN->addIncoming(I, Pred);
 
@@ -194,15 +209,20 @@
     // Rewrite all uses outside the loop in terms of the new PHIs we just
     // inserted.
     for (Use *UseToRewrite : UsesToRewrite) {
-      // If this use is in an exit block, rewrite to use the newly inserted PHI.
-      // This is required for correctness because SSAUpdate doesn't handle uses
-      // in the same block.  It assumes the PHI we inserted is at the end of the
-      // block.
       Instruction *User = cast<Instruction>(UseToRewrite->getUser());
       BasicBlock *UserBB = User->getParent();
+
+      // A PHI may already act as an LCSSA PHI as explained in the gathering
+      // of uses to rewrite (note that here there are possibly other PHI's
+      // included when we added LCSSA PHI's in the exit blocks, after the
+      // gathering of uses)
       if (auto *PN = dyn_cast<PHINode>(User))
         UserBB = PN->getIncomingBlock(*UseToRewrite);
 
+      // If this use is in an exit block, rewrite to use the newly inserted PHI.
+      // This is required for correctness because SSAUpdate doesn't handle uses
+      // in the same block.  It assumes the PHI we inserted is at the end of the
+      // block.
       if (isa<PHINode>(UserBB->begin()) && isExitBlock(UserBB, ExitBlocks)) {
         UseToRewrite->set(&UserBB->front());
         continue;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89739.299163.patch
Type: text/x-patch
Size: 3693 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20201019/78a5fb6d/attachment.bin>