[PATCH] D100464: [DSE] Remove stores in the same loop iteration

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/DeadStoreElimination.cpp:1409
+              LI.getLoopFor(KillingDef->getBlock()) &&
           !IsGuaranteedLoopInvariant(const_cast<Value *>(CurrentLoc->Ptr))) {
+        LLVM_DEBUG(dbgs() << "  ... not guaranteed loop invariant\n");
----------------
nikic wrote:
> I'm not sure this logic is correct. The problem I see is that LoopInfo only tracks natural loops. This means that even though Current and KillingDef might be in the same natural loop (say the nullptr loop), Current might still be part of a (non-natural) cycle, in which case KillingDef may not kill it.
> 
> I'm not particularly familiar with LoopInfo, but that's my understanding of how it works.
> I'm not sure this logic is correct. The problem I see is that LoopInfo only tracks natural loops. This means that even though Current and KillingDef might be in the same natural loop (say the nullptr loop), Current might still be part of a (non-natural) cycle, in which case KillingDef may not kill it.
> 
> I'm not particularly familiar with LoopInfo, but that's my understanding of how it works.


Yes I think that's correct. There might be cycles that LoopInfo does not detect. So we should not rely on comparing the loops on its own. 

If there is no loop for a given block, they still might be in a cycle that has not been detected by LoopInfo due to irreducible control flow; if there's a loop for a given block, it could be in a cycle that's completely contained in the loop we found. If LI finds a loop for a block, I think it is guaranteed that the loop is only entered through the header though.

I think this should allow us to treat the `phis` in the header as being in the same iteration or invariant in any undetected cycles in the loop.



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

https://reviews.llvm.org/D100464