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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Could you please rebase this? The patch doesn't apply cleanly to main. (Insert rant here about Phabricator not providing 3way patches, making it impossible to resolve conflicts when applying.)



================
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");
----------------
dmgreen wrote:
> 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?
I think that's a good way to handle it. Dealing with irreducible control flow in full generality is unlikely to be worthwhile.

Might want to extract this condition into a separate function, as it's getting a bit hard to read...


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

https://reviews.llvm.org/D100464