[PATCH] D137758: [HardwareLoops] remove loops which has undef count

Sam Parker via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 11 01:07:15 PST 2022


samparker added inline comments.


================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:158
     bool MadeChange = false;
+    SmallVector<Loop *, 1> DeadLoops;
   };
----------------
shchenz wrote:
> samparker wrote:
> > nit: I'd prefer that these were references.
> Thanks for review.
> But I am a little confused by this comment, do you mean `SmallVector<Loop &, 1> DeadLoops;`? Seems this can not pass the build
Whooops, sorry, shows I haven't written much C++ recently!


================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:247
+    // Set incoming value to poison for phi nodes in the exit block.
+    for (PHINode &P : ExitBlock->phis()) {
+      std::fill(P.incoming_values().begin(), P.incoming_values().end(),
----------------
shchenz wrote:
> samparker wrote:
> > I see the same logic is used in `LoopDeletion`, so should this be factored out into `deleteDeadLoop`?
> There are two callers for `deleteDeadLoop` in `LoopDeletion`. And for my understanding, these two callers are handling two different scenarios. The first one is like what we do here, the loop is never be executed, so we can just remove the loop and set the phi incoming value from exiting block to poison. And the other one is for loops which act like executing only once, i.e. all instructions in the loop are invariant. So we can make the incoming value from exiting block be invariant and then set it as the new value from preheader after deleting the phi.
> 
> I agree we can handle these two scenarios in `deleteDeadLoop`, but that would require a non-trivial work and not sure it is desirable. For example, we may also need to move some of the logic in `isLoopDead()` to `deleteDeadLoop` as the new incoming value for scenario 2 is calculated there. And for the new incoming value `poison` for scenario 1, we currently calculate just before we call `deleteDeadLoop`.
> 
> Thoughts?
Okay, sounds like more effort than it's worth then.


================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:447
+  // dedicated exit block, so we just delete these loops.
+  if (isa<UndefValue>(Count)) {
+    LLVM_DEBUG(dbgs() << " - Bailing, loop count is undef\n");
----------------
shchenz wrote:
> fhahn wrote:
> > I doubt this is enough. I think you would at least have to ensure that the branch on undef is executed unconditionally in the loop. Also, shouldn't those dead loops be cleaned up earlier?
> > I think you would at least have to ensure that the branch on undef is executed unconditionally in the loop.
> 
> For hardware loops, the exiting block which contains the compare with loop count will be executed in each loop iteration.
> 
> > Also, shouldn't those dead loops be cleaned up earlier?
> 
> OPT has the pass that can clean up this kind of loop. But in LLC pipeline, there is no such pass.
Is there a blocker on adding the pass to the llc pipeline?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137758



More information about the llvm-commits mailing list