[PATCH] D132022: [SCEVExpander] Drop poison-generating flags on hoisting. PR57187

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1056
+    // Drop flags that are potentially inferred from old context.
+    I->dropPoisonGeneratingFlags();
   }
----------------
mkazantsev wrote:
> fhahn wrote:
> > This seems too pessimistic and unnecessarily drops flags that are not inferred based on the context instruction, e.g. because the original IV already had `nuw/nsw`.
> > 
> > Would it be possible to preserve the flags valid at entry of the loop when hoisting?
> We can't do it here. But I guess this is mostly used in IndVars, and we can further strengthen no-wrap flags after this hoisting, if it happened. Didn't try it and don't know the consequences. Can it go as separate patch?
After another thought, shouldn't moving the IV be fine by itself? It might generate poison at the earlier location, but that is fine, as long as it is not used before the original location. in a context where poison triggers UB.

Isn't the real issue the code that later replaces uses of the original IV with the hoisted IV? When updating uses, whether all flags can be retained would depend on whether the UB-on-poison user may execute in cases where the original IV increment didn't?


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

https://reviews.llvm.org/D132022