This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Implement canCreatePoison
ClosedPublic

Authored by aqjune on Apr 10 2020, 12:38 PM.

Details

Summary

This PR adds canCreatePoison(Instruction *I) which returns true if I can generate poison from non-poison
operands.

Diff Detail

Event Timeline

aqjune created this revision.Apr 10 2020, 12:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 10 2020, 12:38 PM
spatel added inline comments.Apr 13 2020, 1:11 PM
llvm/lib/Analysis/ValueTracking.cpp
4603–4604

URem/SRem are not PossiblyExactOperator, so I think this will assert. Please add at least 1 test to confirm.

Would it be better to create a preliminary "canCreatePoison()" helper function that is structured like Instruction::andIRFlags() and return 'false' on any of those instructions?

aqjune updated this revision to Diff 257284.Apr 14 2020, 4:21 AM

Renamed canPushFreezeIntoOperands to canCreatePoison (merged two rather than splitting a helper function, because the two seems to have no difference at this moment).
Used the idiom from Instruction::andIRFlags().
Also renamed isPoisonIf to impliesPoison because the latter seems more formal.

aqjune updated this revision to Diff 257301.Apr 14 2020, 6:05 AM
aqjune marked an inline comment as done.

clang-format

I have enough questions about canCreatePoison() alone that I have not looked at impliesPoison() yet.
That might be a sign that we should split the review into 2 patches, but I think it's ok either way.

llvm/include/llvm/Analysis/ValueTracking.h
596

typo: returnd

597

Add an example of the vector case in this code comment? Is it possible with the current code?

llvm/lib/Analysis/ValueTracking.cpp
4600–4602

We could refine this, right? Ie, if the shift amount is a constant that is less than bitwidth, then shifts can not produce poison (unless they have wrapping/exact flags).
I don't know if that's the only case like this, so it's fine to make a TODO comment for a future patch if that's correct.

4619

plain "cast" rather than "dyn_cast"?

4643

I find it confusing to switch on opcode, check flags, then switch on opcode again. Can we check flags first, and then switch on opcode just once?

4646–4647

Immediate undefined behavior (eg, sdiv %x, 0) is explicitly not poison?
If correct, please note that in the documentation comment for this function, so there is no confusion.
(There's existing code called "MightCreatePoisonOrUB" in instcombine, but I'm not sure if it can be changed to use this new analysis function.)

aqjune updated this revision to Diff 257449.Apr 14 2020, 12:31 PM
  • Leave canCreatePoison only
  • Leave the latter switch only (merged)
  • Let shl/ashr/lshr deal with constant shift amount
  • Address other comments, add more tests
aqjune updated this revision to Diff 257451.Apr 14 2020, 12:33 PM
aqjune marked an inline comment as done.
  • Minor update to a comment
aqjune marked 5 inline comments as done.Apr 14 2020, 12:35 PM
aqjune marked an inline comment as done.Apr 14 2020, 1:30 PM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4646–4647

For MightCreatePoisonOrUB, we can use this function, but it can be broken if canCreatePoison uses deeper analysis.
You mean this optimization when M is given such that shufflevector can work as a vector select:

// shuffle (op X, C0), (op Y, C1), M --> op (shuffle X, Y, M), C'
// shuffle (op C0, X), (op C1, Y), M --> op C', (shuffle X, Y, M)

For example:

X = 0 & (freeze X0)
Y = 0 & (freeze Y0) // X and Y are 0
// canCreatePoison concluded that shl C0, X and shl C1, Y are safe
shuffle (shl C0, X), (shl C1, Y), undef
=>
shl C', (shufflevector X, Y, undef)
=>
shl C', undef

To make using canCreatePoison safe, M should not contain undef elements.

aqjune retitled this revision from [ValueTracking] Implement isPoisonIf and canPushFreezeIntoOperands to [ValueTracking] Implement canCreatePoison.Apr 14 2020, 1:32 PM
aqjune edited the summary of this revision. (Show Details)
spatel accepted this revision.Apr 14 2020, 1:45 PM

LGTM

llvm/include/llvm/Analysis/ValueTracking.h
596–597

This still doesn't read quite right to me. Should it be:
"For vectors, return true if there is potential poison in any element."

This revision is now accepted and ready to land.Apr 14 2020, 1:45 PM

For the impliesPoison one, I made a separate patch here: https://reviews.llvm.org/D78152
But I guess the only place where impliesPoison would be used right now will be the select optimization, then having it in ValueTracking might be too much, so I'm fine with putting it into InstCombine or somewhere else.

aqjune marked an inline comment as done.Apr 14 2020, 1:58 PM
This revision was automatically updated to reflect the committed changes.