Page MenuHomePhabricator

[ValueTracking] Add canCreateUndefOrPoison & let canCreatePoison use Operator
ClosedPublic

Authored by aqjune on Jul 17 2020, 1:51 AM.

Details

Summary

This patch

  • adds canCreateUndefOrPoison
  • refactors canCreatePoison so it can deal with constantexprs

canCreateUndefOrPoison will be used at D83926.

Diff Detail

Event Timeline

aqjune created this revision.Jul 17 2020, 1:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2020, 1:51 AM
jdoerfert added inline comments.Jul 17 2020, 9:20 AM
llvm/include/llvm/Analysis/ValueTracking.h
611

Drive by: Now that I see this, I would prefer the public API to have two functions. I'm fine if others disagree but for the users it seems more natural to have canCreateUndef and canCreateUndefOrPoison, (and maybe canCreatePoison).

llvm/lib/Analysis/ValueTracking.cpp
4791

This seems wrong. A load of an uninitialized location should produce undef for example, shouldn't it? I fear there are way more things that can produce undef. The default should be true here.

nikic added inline comments.Jul 17 2020, 9:25 AM
llvm/include/llvm/Analysis/ValueTracking.h
611

Same here. I would prefer there to only be canCreatePoison and canCreatePoisonOrUndef. Just canCreateUndef by itself would be a footgun, because poison relaxes to undef. I don't see an immediate use for a "can create undef excluding undef that was originally poison" query.

llvm/lib/Analysis/ValueTracking.cpp
4791

Returning true here would make this function always return true :) If we drop the "undef only" mode, then returning false should be fine, as this function only needs to deal with cases where undef is possible but poison is not.

jdoerfert added inline comments.Jul 17 2020, 11:54 AM
llvm/lib/Analysis/ValueTracking.cpp
4791

What about my load example? poison is not possible but undef is, returning false still seems wrong, or maybe I'm just confused..

nikic added inline comments.Jul 17 2020, 12:01 PM
llvm/lib/Analysis/ValueTracking.cpp
4791

Loads can also return poison, and canCreatePoison() will return true for them. (Having load/store implicitly filter poison would prevent mem2reg/SROA.)

jdoerfert added inline comments.Jul 17 2020, 1:09 PM
llvm/lib/Analysis/ValueTracking.cpp
4791

Loads *can* return poison. Some are known *not to* but still *can* return undef. Are we sure this implicit link between the two procedures is wise? I mean, even if the situation I describe doesn't happen now, how do you prevent someone from making canCreatePoison smarter and introducing a bug here?

nikic added inline comments.Jul 17 2020, 1:16 PM
llvm/lib/Analysis/ValueTracking.cpp
4791

Okay, I see your concern now. In that case I would suggest combining these into a single canCreateUndefOrPoison() function with a PoisonOnly flag (for the PoisonChecking consumer). That should make it obvious that these need to be considered as a unit.

jdoerfert added inline comments.Jul 17 2020, 4:29 PM
llvm/lib/Analysis/ValueTracking.cpp
4791

The implementation (=logic) to determine poison and undef should be at the same place. We can expose it via multiple functions but from the perspective of determining it the two are closely connection (IMHO).

aqjune updated this revision to Diff 279065.Jul 19 2020, 12:02 AM
aqjune marked an inline comment as done.

Reflect comments

aqjune marked 4 inline comments as done.Jul 19 2020, 12:04 AM
aqjune added inline comments.
llvm/include/llvm/Analysis/ValueTracking.h
611

Leaving canCreateUndefOrPoison and canCreatePoison only, since it turns out that canCreateUndef could not be used to resolve the instsimplify problem at D83360

aqjune retitled this revision from [ValueTracking] Change canCreatePoison to take Operator, add canCreateUndef to [ValueTracking] Add canCreateUndefOrPoison & let canCreatePoison use Operator.Jul 19 2020, 12:06 AM
aqjune edited the summary of this revision. (Show Details)
aqjune marked 2 inline comments as done.
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4717

Added noundef return attribute check here since this is a pretty small change

nikic accepted this revision.Jul 19 2020, 6:21 AM

LG

llvm/unittests/Analysis/ValueTrackingTest.cpp
813

This might be more obvious if it tested for canCreateUndefOrPoison && !canCreatePoison == Undef.

This revision is now accepted and ready to land.Jul 19 2020, 6:21 AM
jdoerfert accepted this revision.Jul 19 2020, 9:19 AM
jdoerfert added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4773

Thx :)

This revision was automatically updated to reflect the committed changes.