[PATCH] D132022: [SCEVExpander] Recompute 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:
> mkazantsev wrote:
> > mkazantsev wrote:
> > > 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.
> > Unfortunately, in practice it's undoable. This function may hoist multiple instructions, and then the notion that their flags were context-dependent gets lost. If we dropt flags immediately, we can't figure it out later. I propose to stick to this fix and then try to restore flags when possible.
> Actually we have all API available here, so let's just heal the flags.
> Unfortunately, in practice it's undoable. This function may hoist multiple instructions, and then the notion that their flags were context-dependent gets lost

Yeah this is unfortunate indeed! I still think the right place to fix the flags would be where we actually introduce uses that cause UB on poison, but given the difficulty I guess our best option is doing it here. Could you add a comment explaining why this needs to be done here, even though hoisting in itself isn't a problem.


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

https://reviews.llvm.org/D132022