[PATCH] D108107: [LoopFlatten] Fix overflow check

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

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.

In D108107#2946858 <https://reviews.llvm.org/D108107#2946858>, @RosieSumpter wrote:

> 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.



================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:501
+          bool GEPDominatesLatch = false;
+          if (isa<LoadInst>(GEPUser) || isa<StoreInst>(GEPUser))
+            GEPDominatesLatch = isGuaranteedToExecuteForEveryIteration(
----------------
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;


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

https://reviews.llvm.org/D108107