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.
Details
Diff Detail
- Repository
- rL LLVM
- Build Status
Buildable 25390 Build 25389: arc lint + arc unit
Event Timeline
Would it be possible to add a test case?
lib/Analysis/ValueTracking.cpp | ||
---|---|---|
2019 | Do we need this in a separate block? |
Test cases did change but I somehow missed to add them to the diff.
They are attached now.
lib/Analysis/ValueTracking.cpp | ||
---|---|---|
2019 | No, this way the comment relates to all three but I can remove the block if you think it is unnecessary. |
lib/Analysis/ValueTracking.cpp | ||
---|---|---|
2028 | 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 | 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?) |
lib/Analysis/ValueTracking.cpp | ||
---|---|---|
2024 | 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 | I think it would be worth adding a separate file to test the various combinations of supported/unsupported casts. |
lib/Analysis/ValueTracking.cpp | ||
---|---|---|
2028 | +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. |
Ensure no-op or extension casts and add appropriate tests. Also fix an error in LazyValueInfo which looked through AddrSpaceCast to determine non-nullness.
lib/Analysis/ValueTracking.cpp | ||
---|---|---|
2024 | 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? |
lib/Analysis/ValueTracking.cpp | ||
---|---|---|
2024 | Seems ok to me, but if we're going to dyn_cast, you can use 'auto *' to make it less wordy: 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. |
lib/Analysis/ValueTracking.cpp | ||
---|---|---|
2024 | 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. |
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 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. |
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.
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 |
Do we need this in a separate block?