Use nonnull and dereferenceable from an assume bundle in isKnownNonZero
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
708 | I feel we need to move some/all of this complexity into KnowledgeRetention. if (Q.DT && Q.CtxI) { SmallVector<AttributeKind, 2> AttrKinds; AttrKinds.append(Attribute::NonNull); if (llvm::NullPointerIsDefined(Q.CtxI->getFunction(), V->getType()->getPointerAddressSpace())) AttrKinds.append(Attribute::Dereferenceable); if (hasKnownAttributeDominatingInst(*V, AttrKinds, *Q.DT, *Q.CtxI)) return true; } The impl of hasKnownAttributeDominatingInst does the required steps, thus looking at the uses. Actually all the steps should be in getKnownNonNullAndDerefBytesForUse which lives in Attributor.cpp. It does actually more, e.g., use in a call site or and recursive queries via the Attributor, but we should somehow share this logic. My suggestion:
WDYT? |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
708 | Maybe hasKnownAttributeDominatingInst should not need to look for deref at all as we derive it from deref in the Attributor. Otherwise we could make a helper to encapsulate the NullPointerIsDefined stuff. |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
697 | nit: is llvm:: needed? | |
llvm/lib/IR/KnowledgeRetention.cpp | ||
314 ↗ | (On Diff #251015) | There's AssumptionCache, which is used in various places that inspect assumes, including ValueTracking. Is there a reason to not use it here? If there is a layering problem, I think that would need a bit more thought, but ideally we would re-use the existing infrastructure to make things available as widely as possible. |
318 ↗ | (On Diff #251015) | There's isValidAssumeForContext (in ValueTracking.h, https://llvm.org/doxygen/namespacellvm.html#af1d57d70f59588a35ebfe3c4f50fecd2), which already has a similar check (but more general and not necessarily requiring DT. It might be good to use that here instead. |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
708 | i adapted the getKnownNonNullAndDerefBytesForUse to use the same code. but i am not sure this matches what you had in mind.
in ValueTracking this is not needed if the attributor has run before. but i am not sure all places that can add a deref attribute will add a nonnull as well when they can. if you are confident i am fine with not looking for deref. | |
llvm/lib/IR/KnowledgeRetention.cpp | ||
314 ↗ | (On Diff #251015) | isValidAssumeForContext is now used and i saw the improvement in a test case. for AssumptionCache. it could be used here but i think it would be costlier to use it than what we currently have. |
I have minor comments but I think this is fine. Please wait on the approval of @fhahn .
llvm/include/llvm/IR/KnowledgeRetention.h | ||
---|---|---|
136 ↗ | (On Diff #251055) | Return true iff U is a use ... with an attribute in AttrKinds. There is no predicate. |
144 ↗ | (On Diff #251055) | - and this assume dominate the instruction Ctx. The above description matches a convenient wrapper we should add (I also mention that below). |
llvm/lib/Analysis/ValueTracking.cpp | ||
704 | I don't think DT is mandatory here (line 664). | |
708 | This is fine with me. We might want to add a helper that adds this "common" filter (return isValidAssumeForContext(Assume, Q.CxtI, Q.DT);) given an instruction (CtxI) and an optional dom tree. | |
llvm/test/Analysis/ValueTracking/assume.ll | ||
92 | Add test4b, as test4 but with: |
llvm/lib/IR/KnowledgeRetention.cpp | ||
---|---|---|
302 ↗ | (On Diff #251055) | This could be an early exit (like auto *II = dyn_cast....; if (!II) return none(). |
314 ↗ | (On Diff #251015) |
I am not sure how using assumptionCache would be more costly, assuming it is already populated?
But you still have to check all uses of each value (of which many may not be assumptions), to find the available assumptions IIUC. By using AssumptionCache, you can skip the traversal of the use list, which should be quite beneficial, assuming most values do not have associated assumptions. |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
708 | isValidAssumeForContext is in Analysis so we can't access it from KnowledgeRetention. | |
llvm/lib/IR/KnowledgeRetention.cpp | ||
314 ↗ | (On Diff #251015) | when we lookup from use, we can use the operand number so we can use getKnowledgeFromOperandInAssume which is expected to be cheaper than log N for N being the number of element in the assume. if we don't have the operand number, we have to fallback to linear search. that said i think we can be get the best of both world by improving AssumptionCache to store the operand bundle index with the assume. but this should probably go into an other patch. |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
708 | That's unfortunate in terms of layering :( I assume the reason this is in IR is because of the AssumeBuilder part, right? Did you consider splitting the code into an analysis part (that lives in lib/Analysis and can share utilities with ValueTracking & co) & a transform part (that lives in either IR/Transforms)? I think it would be great to share utilities with the existing code that deals with assumes, as that might lead to general improvements that also benefit the existing code, increases test coverage and reduces duplication. | |
llvm/lib/IR/KnowledgeRetention.cpp | ||
314 ↗ | (On Diff #251015) | Right, I see what you mean now. I guess it all depends on what we expect to be larger, the number of (non-assume) uses of the value or the number of operands in the assume. I would expect the former to be larger in practice, given that most values do not have any assume users. |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
708 |
i agree
Yes this is why.
yeah i think this is going to happen. but there are too many constraints to satisfy them all, although we can satisfy more constraint than we currently do.
I think that moving the Query part to Analysis is overall a good idea. |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
708 |
Is that the main reason for being in IR/ currently? I think a better compromise would be to have the AsumeBuilder in Transforms/Utils or something and then test the query part in Transform/ there as well. Also, shouldn't it be also possible to test the query part without the AssumeBuilder, by specifying the expected assume bundles in the IR directly?
+1 to moving the pass out of IR/.
Sounds good. That should be a relatively straight forward change I think.
As mentioned above, if the main reason for it being in IR/ is the unit tests, I would recommend directly construction instruction with assume bundles for the tests in Analysis/ (if possible. I might miss some details here). Conceptually the pass should be somewhere in Transforms/. I think a comparable case is PredicateInfo, which lives in llvm/include/llvm/Transforms/Utils/PredicateInfo.h. |
llvm/lib/Analysis/ValueTracking.cpp | ||
---|---|---|
708 | SGTM, thanks! |
I'm fine with this, one test needs an update though.
llvm/test/Transforms/Attributor/nonnull.ll | ||
---|---|---|
42 | I don't think these test what they should (the change in the Attributor). Since the Attributor knows about assumes (D74888) it can do this deduction already: https://godbolt.org/z/S-PNc- I think this should work as test though: |
llvm/lib/Analysis/AssumeBundleQueries.cpp | ||
---|---|---|
133 | This probably could be simplified by using the stuff from PatternMatch.h, e.g. something like match(U->getUser(), m_Intrinsic<Intrinsic::assume>(m_Specific(U)) | |
147 | nit: llvm:: not needed there I think | |
151 | Can the cache contain elements that are not assumes? I think ideally it would only contain entries with valid assume calls. | |
155 | llvm:: not needed here I think as there is using namespace llvm. | |
llvm/lib/Analysis/ValueTracking.cpp | ||
695 | nit: llvm:: not needed I think |
addressed comments.
add asserts to make sure that both path of getKnowledgeForValue lead to the same result.
llvm/lib/Analysis/AssumeBundleQueries.cpp | ||
---|---|---|
151 | the assumption cache cannot hold valid pointers to non-assume instruction. |
llvm/include/llvm/Analysis/AssumeBundleQueries.h | ||
---|---|---|
18 | Nothing from the header seems to be actually used. Forward declare Instruction? | |
19 | I could not find anything that uses declarations from PassManager.h. Did I miss something? | |
20 | Nothing from the header seems to be actually used. Forward declare DominatorTree? | |
103 | unrelated nit: property that holds? | |
105–109 | unrelated, but it is not clear from the comment what ArgValue is referring to. Might be good to clarify | |
145 | nit: Return | |
146 | nit: period at end of sentence. | |
150 | nit: Return | |
155 | llvm:: not needed? | |
158 | nit: Return | |
llvm/lib/Analysis/AssumeBundleQueries.cpp | ||
133 | It would be slightly better to just use cast<> after the check I think. | |
llvm/lib/Transforms/IPO/Attributor.cpp | ||
19 ↗ | (On Diff #257252) | all new includes unused? |
Thanks for all the changes! The patch looks good to me with respect to my comments, but I think it would be good for someone else to have another look as well.
llvm/include/llvm/Analysis/AssumeBundleQueries.h | ||
---|---|---|
106 | nit: For example ..... of at least for:
| |
llvm/lib/Analysis/ValueTracking.cpp | ||
703 | Just a side note: it would be good to unify the assumption handling for both variants :) |
LGTM. One nit below and the Attributor test needs to be changed (see my 2 comments) and rebased + updated wrt check lines.
llvm/lib/Analysis/AssumeBundleQueries.cpp | ||
---|---|---|
139 | Nit: const auto &AttrKind : AttrKinds) or no auto but the type. Especially the name Attr is (by me) associated with llvm::Attribute not the kind enum. | |
llvm/test/Transforms/Attributor/nonnull.ll | ||
208 | We don't check these prefixes anymore. We need to create a call of test10 to see the return attirbute (I think). |
Nothing from the header seems to be actually used. Forward declare Instruction?