[ValueTracking] Add canCreateUndefOrPoison & let canCreatePoison use Operator

[aqjune]

Summary

This patch

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

canCreateUndefOrPoison will be used at D83926.

[jdoerfert]

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]

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]

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]

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]

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]

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]

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]

Reflect comments

[aqjune]

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]

llvm/lib/Analysis/ValueTracking.cpp

4717–4720

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

[nikic]

LG

llvm/unittests/Analysis/ValueTrackingTest.cpp

813

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

[jdoerfert]

llvm/lib/Analysis/ValueTracking.cpp

4773

Thx :)