This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Add UndefOrPoison/Poison-only version of relevant functions
ClosedPublic

Authored by aqjune on Jul 21 2020, 8:39 AM.

Details

Summary

This patch adds isGuaranteedNotToBePoison and programUndefinedIfUndefOrPoison.

isGuaranteedNotToBePoison will be used at D75808. The latter function is used at isGuaranteedNotToBePoison.

Diff Detail

Event Timeline

aqjune created this revision.Jul 21 2020, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2020, 8:39 AM
aqjune marked an inline comment as done.Jul 21 2020, 8:42 AM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4830–4831

This is a fix to a bug that caused returning false more often. Since it was a small change, embedded here

nikic added inline comments.Jul 21 2020, 9:36 AM
llvm/include/llvm/Analysis/ValueTracking.h
581

As these functions are the same, I would just rename getGuaranteedNonPoisonOp to getGuaranteedNonUndefOrPoisonOp.

llvm/lib/Analysis/ValueTracking.cpp
4791

If PoisonOnly=true, does this actually end up returning true for UndefValue in all cases? It's not completely obvious to me. Maybe if (isa<UndefValue>(C)) return PoisonOnly;?

4800

As a followup, we may want to recursively check the elements instead.

4830–4831

I'd prefer to land this separately with the corresponding test case, otherwise it gets lost in the rest of the patch.

4838

To make sure I get the logic here: Undef is a bit-wise concept and programUndefinedIfUndefOrPoison() tells us whether we get UB for a full undef operand, while isGuaranteedNotToBeUndefOrPoison() tells us whether there can be any undef bits, so we can apply this only for the i1 case. Is that right?

aqjune marked an inline comment as not done.Jul 21 2020, 10:30 AM
aqjune added inline comments.
llvm/include/llvm/Analysis/ValueTracking.h
581

I think splitting these two is good (even if their contents are actually the same) because users like PoisonChecking want to only consider poison values. What do you think?

llvm/lib/Analysis/ValueTracking.cpp
4791

You're right, it shouldn't fall through. Will fix this

4800

Sounds good!

4830–4831

Okay, will make a separate patch for this

4838

Yes, exactly.

aqjune updated this revision to Diff 279704.Jul 21 2020, 9:53 PM

Fix undef case

aqjune marked 2 inline comments as done.Jul 21 2020, 9:54 PM
aqjune updated this revision to Diff 280706.Jul 25 2020, 11:55 PM

clang-format

aqjune updated this revision to Diff 281299.Jul 28 2020, 11:08 AM

Rebase,
Remove getGuaranteedNonUndefOrPoisonOp since getGuaranteedNonUndefOrPoisonOp and getGuaranteedNonPoisonOp are equivalent

aqjune added inline comments.Jul 28 2020, 11:10 AM
llvm/include/llvm/Analysis/ValueTracking.h
581

Leave getGuaranteedNonPoisonOp only & left a comment at previous getGuaranteedNonUndefOrPoisonOp's uses saying that non-poison op and non-undef op are equivalent

I think I'm fine with this, though it's a lot. @nikic ?

llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp
296

Interesting that we need these.

aqjune updated this revision to Diff 290397.Sep 7 2020, 7:53 PM

Rebase, and have a few changes to maximally utilize noundef attributes.
programUndefinedIfUndefOrPoison(I) is updated in a way that it returns true
if I is partially undef.
For example, the updated programUndefinedIfUndefOrPoison(I) returns true in this case:

%I = or i32 x, 1 ; if x = undef, %I is partially undef
call void @f(i32 noundef %I)
nikic accepted this revision.Sep 8 2020, 2:23 PM

LGTM

llvm/unittests/Analysis/ValueTrackingTest.cpp
734

avlue -> value

This revision is now accepted and ready to land.Sep 8 2020, 2:23 PM
aqjune edited the summary of this revision. (Show Details)Sep 9 2020, 3:59 AM
This revision was landed with ongoing or failed builds.Sep 9 2020, 4:01 AM
This revision was automatically updated to reflect the committed changes.