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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

> Thanks for the patch! Given that this only shifts the invocation of LI, there shouldn't be any problems in terms of compile time.

It should iterate the same amount too, so I'm hoping it's OK. (But I have no evidence one way of the other).



================
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");
----------------
fhahn wrote:
> 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.
> 
Ah, excellent point. I had not considered irreducible loops and they are not the kind of thing that naturally comes up in testing. (Unfortunately I feel like all the testing I could try would already have been run by Florian before DSE was committed, and it didn't show anything then just as it would not show any problems now. csmith didn't feel like much help). I was originally more worried about opening the door to any number of unrelated issues coming up from allowing DSE from different blocks where both LI's return nullptr.

For the moment, I have added a check using mayContainIrreducibleControl on the function and bailing if there are any.  What do you think? Too much of a blunt hammer, or an OK way to handle this?


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

https://reviews.llvm.org/D100464