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

[ChenZheng via Phabricator via llvm-commits]

shchenz marked 3 inline comments as done.
shchenz added inline comments.


================
Comment at: llvm/lib/CodeGen/HardwareLoops.cpp:158
     bool MadeChange = false;
+    SmallVector<Loop *, 1> DeadLoops;
   };
----------------
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


================
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(),
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137758