Page MenuHomePhabricator

[ValueTracking] Look through casts when determining non-nullness
ClosedPublic

Authored by jdoerfert on Nov 27 2018, 9:49 AM.

Details

Summary

Bitcast, address space cast and ptr2int instructions will not alter the
value of their operand and can therefore be looked through to determine
non-nullness.

Diff Detail

Repository
rL LLVM

Event Timeline

jdoerfert created this revision.Nov 27 2018, 9:49 AM
fhahn added a comment.Nov 27 2018, 9:57 AM

Would it be possible to add a test case?

lib/Analysis/ValueTracking.cpp
2019 ↗(On Diff #175515)

Do we need this in a separate block?

jdoerfert edited the summary of this revision. (Show Details)

Add impacted test cases

Would it be possible to add a test case?

Test cases did change but I somehow missed to add them to the diff.
They are attached now.

jdoerfert marked an inline comment as done.Nov 27 2018, 10:02 AM
jdoerfert added inline comments.
lib/Analysis/ValueTracking.cpp
2019 ↗(On Diff #175515)

No, this way the comment relates to all three but I can remove the block if you think it is unnecessary.

jdoerfert marked an inline comment as done.

Look through even more instructions

fhahn added inline comments.Nov 27 2018, 10:40 AM
lib/Analysis/ValueTracking.cpp
2028 ↗(On Diff #175527)

Not sure if that is safe, according to LangRef: The ‘addrspacecast’ instruction converts the pointer value ptrval to type pty2. It can be a no-op cast or a complex value modification, depending on the target and the address space pair.

2031 ↗(On Diff #175527)

ptrtoint & inttoptr do truncate if the target type is smaller than the value type. Wouldn't it be possible that this changes a non-zero value to a zero value ( if we truncate all non-zero bits?)

jdoerfert marked 2 inline comments as done.Nov 27 2018, 10:49 AM

I'm also running the LLVM-TS to check nothing breaks.

lib/Analysis/ValueTracking.cpp
2028 ↗(On Diff #175527)

Agreed, I'll remove this again.

2031 ↗(On Diff #175527)

Agreed again, I'll add size checks for these.

fhahn added inline comments.Nov 27 2018, 11:44 AM
lib/Analysis/ValueTracking.cpp
2024 ↗(On Diff #175527)

The dyn_casts are not strictly necessary I think and using a switch on the opcode might be a little bit more compact, together with getOperand(0). But it doesn't really make a big difference.

2028 ↗(On Diff #175527)

I think it would be worth adding a separate file to test the various combinations of supported/unsupported casts.

spatel added inline comments.Nov 27 2018, 1:32 PM
lib/Analysis/ValueTracking.cpp
2028 ↗(On Diff #175527)

+1. If we almost got the logic wrong in this patch, we should have some negative tests to make sure someone doesn't come here in the future and extend it wrongly. A code comment would be good too.

jdoerfert updated this revision to Diff 175572.Nov 27 2018, 1:42 PM
jdoerfert marked 3 inline comments as done.

Ensure no-op or extension casts and add appropriate tests. Also fix an error in LazyValueInfo which looked through AddrSpaceCast to determine non-nullness.

jdoerfert marked 4 inline comments as done.Nov 27 2018, 1:44 PM
jdoerfert added inline comments.
lib/Analysis/ValueTracking.cpp
2024 ↗(On Diff #175527)

The switch is not much nicer, I think. Especially since I don't know how to add the operators, e.g., constant bitcast expressions, in the switch. Does anybody feel strongly about this?

spatel added inline comments.Nov 27 2018, 1:59 PM
lib/Analysis/ValueTracking.cpp
2024 ↗(On Diff #175527)

Seems ok to me, but if we're going to dyn_cast, you can use 'auto *' to make it less wordy:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

Another option to shrink the code would be something like this:

Value *X;
if (match(II, m_CombineOr(m_ZExtOrSExt(m_Value(X)), m_BitCast(m_Value(X)))))
  return isKnownNonZero(X, Depth, Q);

We're missing an m_IntToPtr() matcher...if we added that, all of the cases could use matchers.

jdoerfert added inline comments.Nov 27 2018, 2:04 PM
lib/Analysis/ValueTracking.cpp
2024 ↗(On Diff #175527)

I can do auto * and I can try to implement it in a switch but I don't feel qualified to use the matcher logic just now. We also would need to compare "sizes" for int2ptr/ptr2int.

jdoerfert marked an inline comment as done.Nov 27 2018, 3:15 PM
jdoerfert added inline comments.
lib/Analysis/LazyValueInfo.cpp
1714 ↗(On Diff #175572)

I'm not sure this is the right fix but I know we have to do something to pass "callsite_nonnull_args_through_casts.ll" with -O3 (even without my modifications below!).
The alternative, making sure Value::stripPointerCasts() is not stripping of AddrSpaceCasts, makes more and more sense to me as I always assumed it not to change "the value".

The changes in ValueTracking.cpp look good to me, I am just adding Eli in case he has any additional thoughts.

lib/Analysis/LazyValueInfo.cpp
1714 ↗(On Diff #175572)

I think this is an independent issue and should be addressed separately, assuming the test passes with instcombine.

jdoerfert updated this revision to Diff 175700.Nov 28 2018, 8:30 AM
jdoerfert marked 6 inline comments as done.

Use auto * and remove obsolete modification

jdoerfert added inline comments.Nov 28 2018, 8:30 AM
lib/Analysis/LazyValueInfo.cpp
1714 ↗(On Diff #175572)

Agreed, I'll revert the change. This should be part of the discussion about stripPointerCasts and its tendency to look through AddrSpaceCasts.

lib/Analysis/ValueTracking.cpp
2024 ↗(On Diff #175527)

I use auto * in the next version.

fhahn accepted this revision.Nov 28 2018, 9:28 AM

LGTM, but please wait a bit with committing in case there are additional thoughts.

This revision is now accepted and ready to land.Nov 28 2018, 9:28 AM
efriedma added inline comments.Nov 28 2018, 11:27 AM
lib/Analysis/ValueTracking.cpp
2034 ↗(On Diff #175700)

This will never trigger: a ptrtoint produces an int, but the code is guarded by V->getType()->isPointerTy().

2042 ↗(On Diff #175700)

zext/sext produces an int, but this is guarded by V->getType()->isPointerTy(). And we check for zext/sext a few lines later anyway.

jdoerfert updated this revision to Diff 175768.Nov 28 2018, 2:06 PM

Code movement and removal (based on the comments by Eli Friedman)

Removed s/zext, moved ptr2int to make sense. Bitcast was my actual use case, int2ptr was not (always) covered before, the ptr2int test case was but only late in computeKnownBits.

fhahn added a comment.Jan 25 2019, 3:38 AM

It looks like this patch is good to go in :) Are you planning on committing it soon?

lebedev.ri added inline comments.
test/Transforms/InstCombine/callsite_nonnull_args_through_casts.ll
1 ↗(On Diff #175768)

Pretty much all new instcombine tests use ./utils/update_test_checks.py

This revision was automatically updated to reflect the committed changes.