This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Let isGuaranteedNotToBeUndefOrPoison look into more constants/instructions
ClosedPublic

Authored by aqjune on Mar 11 2020, 10:56 AM.

Details

Summary

This patch helps isGuaranteedNotToBeUndefOrPoison look into more constants and instructions (bitcast/alloca/gep/fcmp).

To deal with bitcast, Depth is added to isGuaranteedNotToBeUndefOrPoison.

This patch is splitted from https://reviews.llvm.org/D75808.

Checked with Alive2

Diff Detail

Event Timeline

aqjune created this revision.Mar 11 2020, 10:56 AM
jdoerfert added inline comments.Mar 14 2020, 6:59 PM
llvm/lib/Analysis/ValueTracking.cpp
4708–4723

misplaced comment

4712

This is not 100% the idea behind stripPointerCastsSameRepresentation:
stripPointerCastsSameRepresentation says the bit-representation stays the same, not that there are no addrspacecasts stripped. If we assume an addressspacecast that is known not to modify the bit-representation will also not modify the poison/undef state, which I think is reasonable, we can do tihs.

aqjune updated this revision to Diff 250399.Mar 14 2020, 8:11 PM

Update comments

aqjune marked 4 inline comments as done.Mar 14 2020, 8:14 PM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4708–4723

Added more text to the comment, my underlying idea was that null was also allowed in this case as well.

4712

Yes, I think in that case undef/poison should be preserved. LangRef says that addrspacecast can be no-op, I believe this function is dealing with the case.
Updated comment

aqjune marked 2 inline comments as done.Mar 14 2020, 8:15 PM
jdoerfert added inline comments.Mar 14 2020, 11:46 PM
llvm/lib/Analysis/ValueTracking.cpp
4708–4723

I still find it misplaced (and I dislike comments in a single conditional stmt without braces). Where does the GEP come from and why is it inbounds?

4712

As I said, the function will strip (any) casts that are known not to change the bit representation. I agree that this should be fine here but the comment makes it sounds as if only some addrspacecasts are stripped.

aqjune updated this revision to Diff 250406.Mar 15 2020, 12:33 AM

Update comments

aqjune updated this revision to Diff 250407.Mar 15 2020, 12:34 AM

Minor fix

aqjune marked an inline comment as done.Mar 15 2020, 12:35 AM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4712

Made the comment clearer & clarify why gep inbounds was mentioned here

I don't see a problem in the logic but we need more tests, see below.

llvm/test/Transforms/InstSimplify/freeze.ll
270

We need more (negative) tests that verify we don't do thing we shouldn't. Optimally one for each restriction we have, e.g., gep non-inbounds with potential undef/poison operand (one for base one for index).

reames requested changes to this revision.Mar 16 2020, 3:22 PM
reames added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4724–4725

We should have the same recursion here with the depth limit to handle cycles.

4742

As a follow up, it would be nice to re-write this in terms of existing logic. For example, the operand handling part can be done as:
if (propagatesFullPoison(&I) && std::all_of(I.operands(), OpCheck)) return true;

You do need to filter instructions with poison producing flags above that. For that, you could possible factor out a function "localProducesPoison(Op)" based on the function generatePoisonChecks from PoisonChecking.cpp

This is optional follow on.

This revision now requires changes to proceed.Mar 16 2020, 3:22 PM
aqjune updated this revision to Diff 250701.Mar 17 2020, 12:32 AM

Address comments

aqjune marked 4 inline comments as done.Mar 17 2020, 12:33 AM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4742

Yes, I agree having it would be good. I'll make a follow-up patch for this.

aqjune marked an inline comment as done.Mar 17 2020, 12:35 AM

I think I'm fine with this, wait for @reames to accept though.

llvm/lib/Analysis/ValueTracking.cpp
4722

Nit: No braces.
Use OpCheck for BitCastInst as well.

aqjune updated this revision to Diff 251513.Mar 19 2020, 5:22 PM

Use OpCheck for BitCastInst, aggregate cases that only need operand checks

aqjune updated this revision to Diff 251826.Mar 21 2020, 12:49 AM

Support ConstantDataVector too (needed by https://reviews.llvm.org/D76483)

spatel added a subscriber: spatel.Mar 22 2020, 8:22 AM

This has already been reviewed by others, so if we are close to approval, it's fine...
But if I were starting this patch/review from scratch, I'd prefer that we add all of the tests with baseline CHECK lines as a preliminary commit, and then build the logic up in the code to improve those tests one-by-one.
IIUC, we can make the vector constant change independently of the recursion logic, so that part can be split off into a small separate patch?

llvm/include/llvm/Analysis/ValueTracking.h
605

Is there some reason we can't use the typical ValueTracking construct here?
We usually set default Depth = 0, and then increment up to "const unsigned MaxDepth = 6". We want to avoid making that "6" any more magical than it already is. :)

aqjune updated this revision to Diff 251943.Mar 22 2020, 10:16 PM

Remove ConstantVector check, update test diff

aqjune updated this revision to Diff 251944.Mar 22 2020, 10:21 PM

Make Depth increment from 0 to MaxDepth

aqjune marked 2 inline comments as done.Mar 22 2020, 10:22 PM

@spatel does this look better?
@reames you requested changes, we'll wait for you.

@spatel does this look better?

Yes - I added a few more inline nits. I don't have a good understanding of the casting part of the patch, but other than that LGTM.

llvm/lib/Analysis/ValueTracking.cpp
4737–4738

nit: "auto *"

4741

nit: "auto *"

4766–4767

nit: "auto *"

4767

Use "I" in all of these clauses for consistency.
If the intent is to grow this list, might want to use a "switch(I->getOpcode())" already so later patches are minimal.

aqjune updated this revision to Diff 252285.Mar 24 2020, 5:41 AM

Use switch

aqjune updated this revision to Diff 252293.Mar 24 2020, 6:09 AM

clang-format

aqjune marked 4 inline comments as done.Mar 24 2020, 6:10 AM
jdoerfert added inline comments.Mar 24 2020, 10:34 PM
llvm/lib/Analysis/ValueTracking.cpp
4722

What does " To guarantee that the result isn't poison, the stripped pointer should be checked whether it is inbounds." mean? You cannot/should bot check the stripped value, e.g. determine how we got here.

aqjune marked an inline comment as done.Mar 24 2020, 11:21 PM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4722

What does " To guarantee that the result isn't poison, the stripped pointer should be checked whether it is inbounds." mean?

It means the stripped pointer should be an in-bounds address of an allocated object, not that it should be gep inbounds again ( .. if the base pointer is not an in bounds address of an allocated object, LangRef's getelementptr instruction).

aqjune updated this revision to Diff 252506.EditedMar 24 2020, 11:23 PM

Minor update to a comment to clarify its meaning

spatel added inline comments.Mar 25 2020, 6:40 AM
llvm/test/Transforms/InstSimplify/freeze.ll
21–22

Please remove the TODO comments here and below.
And we could still separate the constant improvements into another patch to make incremental progress...

jdoerfert added inline comments.Mar 25 2020, 8:06 AM
llvm/lib/Analysis/ValueTracking.cpp
4722

I think what you try to say is something along the lines of:

"To guarantee that the result isn't poison, the stripped pointer is checked as it has to be pointing into an allocated object or be null null to ensure inbounds getelement pointers with a zero offset could not produce poison."

aqjune updated this revision to Diff 253036.Mar 26 2020, 8:29 PM

Address comments

aqjune marked 2 inline comments as done.Mar 26 2020, 8:36 PM

Is there any other change needed?
reames's comment (recursively calling the function with an increased Depth) is addressed too.

jdoerfert accepted this revision.Mar 26 2020, 10:28 PM

I'm fine with this. @reames should probably say something.

Polite ping for @reames again - I'll be committing this on Friday.

This revision was not accepted when it landed; it landed in state Needs Review.Apr 25 2020, 7:56 AM
This revision was automatically updated to reflect the committed changes.