[PATCH] D86737: [Attributor] Fix AANoUndef identification

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/test/Transforms/Attributor/IPConstantProp/PR16052.ll:20
 ; IS__CGSCC____-NEXT:  entry:
-; IS__CGSCC____-NEXT:    [[CALL2:%.*]] = call i64 @fn1(i64 undef)
+; IS__CGSCC____-NEXT:    [[CALL2:%.*]] = call i64 @fn1(i64 noundef undef)
 ; IS__CGSCC____-NEXT:    ret i64 [[CALL2]]
----------------
okura wrote:
> okura wrote:
> > jdoerfert wrote:
> > > Hm,... I think this exposes a problem. Could you take a look why we get a noundef here?
> > I overlooked this problem, thanks.
> > In this case, I expected that we indicate a pessimistic fixpoint for `%div = sdiv ...` in `genericValueTraversal` because we cannot strip value even once.
> > A simplified value for `%div` does not have value (i.e. `llvm::None`) because the assumed range is empty. (we can intentionally reduce undef to 0 there.)
> > Therefore, a callback is not called in `genericValueTraversal` and it returns `true`. That's why we got a noundef here.
> > We can avoid this by changing `manifest` not to manifest for such positions.
> I made a new patch for this. D86835
> However, I'm worried that a similar problem will emerge again when another AA has the possibility to change values to undef.
> I think implementing a new interface of Attributor to query if a position will be changed to undef is one of the possible solutions. But it would not be trivial change because we should manage the order of manifestation. (i.e. we should process noundef manifestation at last.)
> How do you think about this?
It seems I did not properly read what was happening. `sdiv .., undef` is UB. So deriving `noundef` for the result is OK. Eventually we should replace this with `unreachable` but not here.


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

https://reviews.llvm.org/D86737