This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Fix AANoUndef identification
ClosedPublic

Authored by okura on Aug 27 2020, 2:15 PM.

Details

Summary

Even though noundef IR attribute might be attached to non-void type values, AANoUndef is mistakenly identified for pointer type values only.
This patch fixes that.

Diff Detail

Event Timeline

okura created this revision.Aug 27 2020, 2:15 PM
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
okura requested review of this revision.Aug 27 2020, 2:15 PM
jdoerfert added inline comments.Aug 27 2020, 5:32 PM
llvm/test/Transforms/Attributor/IPConstantProp/PR16052.ll
20

Hm,... I think this exposes a problem. Could you take a look why we get a noundef here?

okura added inline comments.Aug 29 2020, 2:32 AM
llvm/test/Transforms/Attributor/IPConstantProp/PR16052.ll
20

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.

okura added inline comments.Aug 29 2020, 3:01 AM
llvm/test/Transforms/Attributor/IPConstantProp/PR16052.ll
20

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?

jdoerfert accepted this revision.Aug 29 2020, 10:27 AM

LGTM

llvm/test/Transforms/Attributor/IPConstantProp/PR16052.ll
20

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.

This revision is now accepted and ready to land.Aug 29 2020, 10:27 AM
okura updated this revision to Diff 288793.Aug 29 2020, 11:22 AM

Reflect change in D86835

okura updated this revision to Diff 288804.Aug 29 2020, 1:33 PM

fix OpenMP/parallel_deletion.ll

This revision was automatically updated to reflect the committed changes.