Page MenuHomePhabricator

[LoopFlatten] Fix overflow check
ClosedPublic

Authored by RosieSumpter on Aug 16 2021, 1:21 AM.

Details

Summary

Amended the GEP check in checkOverflow to ensure that the GEP
dominates the loop latch. If the GEP doesn't dominate the latch
then it is not necessarily calculated on each loop iteration, so
the assumption that the GEP will overflow (and be UB) before the
IV no longer holds.

Diff Detail

Event Timeline

RosieSumpter created this revision.Aug 16 2021, 1:21 AM
RosieSumpter requested review of this revision.Aug 16 2021, 1:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 16 2021, 1:21 AM

I think this needs to check that the GEP is used by a load/store that is executed on each iteration.

Can this use one of the isGuaranteedToExecute function too? There is a isGuaranteedToExecuteForEveryIteration, but it does not look very powerful. It may be OK for most loops though, where checking the inner loop header is enough.

Changed the check to see if the GEP is used by a load/store instruction
and, if so, isGuaranteedToExecuteForEveryIteration is used with the instruction and
inner loop to determine if the GEP dominates the latch.

I think this needs to check that the GEP is used by a load/store that is executed on each iteration.

Can this use one of the isGuaranteedToExecute function too? There is a isGuaranteedToExecuteForEveryIteration, but it does not look very powerful. It may be OK for most loops though, where checking the inner loop header is enough.

Hopefully this change is what you had in mind - I'm just not sure whether the

if (isa<LoadInst>(GEPUser) || isa<StoreInst>(GEPUser))

is necessary (can a GEP be used by something that isn't a store or a load?)

Yeah, sounds good to me. There is quite a bit of nesting, but I hope it will be rare to get into it, or quick when it does usually finding the UB on the first item it visits.

Hopefully this change is what you had in mind - I'm just not sure whether the

if (isa<LoadInst>(GEPUser) || isa<StoreInst>(GEPUser))

is necessary (can a GEP be used by something that isn't a store or a load?)

Yeah sounds good. Can you add one extra check that the Store is using the GEP for the address, not the stored value. So it is operand #1, not #0.

llvm/lib/Transforms/Scalar/LoopFlatten.cpp
501

It may be better to reverse the condition so that we reduce the indentation and don't require GEPDominatesLatch any more.

if (!isa<Load>(..) && !isa<Store>(..))
  continue;
if (!isGuaranteedToExecuteForEveryIteration(..))
  continue;
  • Added check that GEP is used as address of store instruction
  • Reversed logic in some places and removed GEPDominatesLatch
RosieSumpter marked an inline comment as done.Aug 16 2021, 9:28 AM
dmgreen accepted this revision.Aug 17 2021, 12:46 AM

LGTM. Thanks

This revision is now accepted and ready to land.Aug 17 2021, 12:46 AM
This revision was landed with ongoing or failed builds.Aug 19 2021, 5:20 AM
This revision was automatically updated to reflect the committed changes.