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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/ScalarEvolutionExpander.cpp:1056
+    // Drop flags that are potentially inferred from old context.
+    I->dropPoisonGeneratingFlags();
   }
----------------
fhahn wrote:
> 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?
Yes, I agree that moving is fine. The bug appears when we introduce a new use (in this case a trunc) to the hoisted IV in a point where the old IV didn't have any uses. I'll try to make a more targeted fix.


================
Comment at: llvm/test/Transforms/IndVarSimplify/X86/pr57187.ll:39
   %iv.zext = zext i32 %iv to i64
   %gep = getelementptr inbounds i64, i64 addrspace(1)* undef, i64 %iv.zext
   %iv.next = add i32 %iv, -1
----------------
fhahn wrote:
> using an `addresspace(1)` pointer here has the unfortunate side effect that `Alive2` cannot handle the code. It would probably be good to remove at least `addrspace(1)` here and maybe even better to just use a function call as use.
Ok, I will update the test.


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

https://reviews.llvm.org/D132022