This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Check uses of Argument if it is given to isGuaranteedNotToBeUndefOrPoison
ClosedPublic

Authored by aqjune on Sep 24 2020, 2:56 AM.

Details

Summary

This is a patch that allows isGuaranteedNotToBeUndefOrPoison to return more precise result
when an argument is given, by looking through its uses at the entry block (and following blocks as well, if it is checking poison only).

This is useful when there is a function call with noundef arguments at the entry block.

Diff Detail

Event Timeline

aqjune created this revision.Sep 24 2020, 2:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 24 2020, 2:56 AM
aqjune requested review of this revision.Sep 24 2020, 2:56 AM
aqjune edited the summary of this revision. (Show Details)Sep 24 2020, 2:56 AM
nikic added inline comments.Sep 24 2020, 10:10 AM
llvm/lib/Analysis/ValueTracking.cpp
4889

I would prefer to move the programUndefinedIfUndefOrPoison() call below outside the Instruction check, and just return false from programUndefinedIfUndefOrPoison() on non-Instruction/Argument (aka Constant).

aqjune added inline comments.Sep 24 2020, 12:01 PM
llvm/lib/Analysis/ValueTracking.cpp
4889

Would it make programUndefinedIfUndefOrPoison() differentiate non-undef and undef constants?
We'll want to make programUndefinedIfUndefOrPoison(undef) return false.

nikic added inline comments.Sep 24 2020, 12:07 PM
llvm/lib/Analysis/ValueTracking.cpp
4889

I don't think it's useful to make a distinction, we can just return false for all constants.

aqjune updated this revision to Diff 294138.Sep 24 2020, 12:40 PM

Address comments

nikic accepted this revision.Sep 24 2020, 12:43 PM

LG with some nits

llvm/lib/Analysis/ValueTracking.cpp
4874

InstOrArg -> V

5170

I think it would be more elegant to write

} else if (auto *Arg = dyn_cast<Argument>(V)) {
   // ...
} else {
   return false;
}

below, rather than having having to keep the checks here and below in sync.

This revision is now accepted and ready to land.Sep 24 2020, 12:43 PM
aqjune added inline comments.Sep 24 2020, 12:43 PM
llvm/lib/Analysis/ValueTracking.cpp
5170

Unit tests say V can be MetadataAsValue as well (that is called from AANoUndefImpl::initialize).
But, does it imply that AANoUndefImpl::initialize should be fixed?

nikic added inline comments.Sep 24 2020, 1:49 PM
llvm/lib/Analysis/ValueTracking.cpp
5170

Unit tests say V can be MetadataAsValue as well (that is called from AANoUndefImpl::initialize).
But, does it imply that AANoUndefImpl::initialize should be fixed?

I don't think there's anything wrong with. It shouldn't cause issues for this code as implemented, right?

This revision was landed with ongoing or failed builds.Sep 24 2020, 4:58 PM
This revision was automatically updated to reflect the committed changes.
aqjune added inline comments.Sep 24 2020, 5:03 PM
llvm/lib/Analysis/ValueTracking.cpp
5170

It happened because there was a function call that had a metadata as its argument, and AANoUndefImpl was somehow trying to work with the metadata argument.
Yes, I think AANoUndefImpl is okay. I feel it might be safe for isGuaranteedNotToBeUndefOrPoison to simply return false when MetadataAsValue is given, to make possible future changes assertion-safe.

aqjune added inline comments.Sep 24 2020, 5:52 PM
llvm/lib/Analysis/ValueTracking.cpp
5170

Made the change at https://github.com/llvm/llvm-project/commit/92106641ae297c24877085e0357e8095aa7b43c9 as a NFC commit, because it's just a couple of lines change