Page MenuHomePhabricator

[ValueTracking] Let isGuaranteedNotToBeUndefOrPoison look into more constants/instructions
Needs ReviewPublic

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

Details

Reviewers
reames
jdoerfert
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.Wed, Mar 11, 10:56 AM
jdoerfert added inline comments.Sat, Mar 14, 6:59 PM
llvm/lib/Analysis/ValueTracking.cpp
4622

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.

4625

misplaced comment

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

Update comments

aqjune marked 4 inline comments as done.Sat, Mar 14, 8:14 PM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4622

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

4625

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

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

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.

4625

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?

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

Update comments

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

Minor fix

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

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.Mon, Mar 16, 3:22 PM
reames added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4633–4634

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

4651

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.Mon, Mar 16, 3:22 PM
aqjune updated this revision to Diff 250701.Tue, Mar 17, 12:32 AM

Address comments

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

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

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

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

llvm/lib/Analysis/ValueTracking.cpp
4639

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

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

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

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

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

spatel added a subscriber: spatel.Sun, Mar 22, 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
604

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.Sun, Mar 22, 10:16 PM

Remove ConstantVector check, update test diff

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

Make Depth increment from 0 to MaxDepth

aqjune marked 2 inline comments as done.Sun, Mar 22, 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
4646–4647

nit: "auto *"

4650

nit: "auto *"

4675–4676

nit: "auto *"

4676

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.Tue, Mar 24, 5:41 AM

Use switch

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

clang-format

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

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.Tue, Mar 24, 11:21 PM
aqjune added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4639

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.EditedTue, Mar 24, 11:23 PM

Minor update to a comment to clarify its meaning

spatel added inline comments.Wed, Mar 25, 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.Wed, Mar 25, 8:06 AM
llvm/lib/Analysis/ValueTracking.cpp
4639

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.Thu, Mar 26, 8:29 PM

Address comments

aqjune marked 2 inline comments as done.Thu, Mar 26, 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.Thu, Mar 26, 10:28 PM

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