This patch introduces a new abstract attributor instance that propagates
assumption information from functions. Conceptually, if a function is
only called by functions that have certain assumptions, then we can
apply the same assumptions to that function. This problem is similar to
calculating the dominator set, but the assumptions are merged instead of
nodes.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
llvm/include/llvm/IR/Assumptions.h | ||
---|---|---|
60 | Consider passing the set as reference, unsure how much work copying it is | |
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
4749 | I don't like the pair approach for long lived data. Make it a struct with properly named members. Costs the same but is much easier to read. | |
4754 | ||
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
15 | Style: move down to the block of includes | |
9703 | This should be in the constructor. Initialize can and should already look at the underlying value. | |
9756 | ||
9759 | ||
9764 | Does this overwrite an existing assumptions attribute? Do we have a test for that? | |
9766 | Adding assumptions is a change. Maybe return if it's empty with unchanged and otherwise add + CHANGED. | |
9769–9772 | Not needed. If you just redirect to the base class, remove it. | |
9779 | No this, also elsewhere. | |
9785 | getIntersection returns if a change was made. CallSitePred should return false only if the traversal of call sites failed and we should give up. I think the return value of the CB here should be: | |
9791 | This is not how checkForAllXXXX work. If they return false it means no all XXXX have been visited and you cannot rely on the information gathered. | |
9798 | This won't be necessary because checkForAllCallSites returns false if you asked for *all* and not all call be visited. | |
9802 | This is invariant, so it should happen in the initialize. | |
9841 | Same comments as in the other manifest. | |
llvm/test/Transforms/Attributor/assumes_info.ll | ||
3 | Use the same run lines as other tests (4 total) |
llvm/include/llvm/IR/Assumptions.h | ||
---|---|---|
60 | Passing it as a reference would require some external lifetime right? Copy elision should apply in this case, but I'm not sure. | |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
9764 | Made a function that always appends assumptions. | |
9802 | The set at some points will still be in the universal state, we only want to add this attribute once it has some known values. |
llvm/lib/Transforms/IPO/Attributor.cpp | ||
---|---|---|
2499 | We need to initialize this for call sites too, at least if they are not direct call sites. | |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
9683 | ||
9686 | I would check the cheap part first, so isvalidstate. | |
9706 | This makes it sound like a push-scheme while the updatImpl uses a pull-scheme. | |
9743 | If the state has "known" and "assumed" assumptions (see also below) you can always fallback to the known ones if you indicate a pessimistic fixpoint. You basically reimplemented parts of the inidicatePessimistic/OptimisticFixpoint stuff here but only partially as part of the state. In line 9742 you do Assumed = Known, which is the definition of indicatePessimisticFixpoint in the integer and boolean state. The special case (line 9737/8) is not necessary at all as Known == Assume == empty in that case anyway. | |
9755 | This is basically the "known assumptions" set while Assumptions is the "assumed assumptions" set. Maybe use those terms to match the rest of the AAs. We might even need to provide two APIs to query a known and an assumed assumption. | |
9788 | I doubt assumptions.empty() means something changed. If you return changed and nothing changed you just force a timeout after the max iteration count. |
Rewriting to use standard state encoding semantics. This currently crashes for
the test case provided when using the cgscc pass for some reason.
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
2524 | Unsure if I'd call this "UniversalSet" as it might not be a universal set ;) | |
2588 | Why would it not be valid if assumed is not universal anymore? | |
2624 | You should not remove any known assumptions, so remove known from RHS first. | |
2642 | We should not cache "changed" anywhere. | |
llvm/lib/IR/Assumptions.cpp | ||
109 | The two functions above are equal, make it a local template function and call it from the two exposed ones. | |
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
9734 | You can/should query the call site AA here. Through inlining and such the call site might have more assumptions than the caller. | |
9750 | The way this is set up it might signal change even if things did not change. That is bad as it will spin until the iteration limit is reached. Either consider known in the intersect or keep track of the number of assumptions you had before and have now. | |
9762 | Technically we can add the caller assumptions and the callee assumptions to the known set here if we want. Not needed right now though. | |
9764 | No need to union something into assumed which is universal at this point. | |
9779 | You can add the Assumed here. We are in a fixpoint state but the AA above might not have been informed of that yet. | |
9793 | We should not look at the callee but caller. The callee is known and asked for "hasAssumption" queries anyway. The caller is what might disappear while the call site sticks around. So propagation direction is "caller -> call site -> callee". | |
9795 | This doesn't actually update the state of this AA at all. You want to add the caller assumptions to the call site and indicate a change if that caused the call site to change. |
The CallBase instance should inherit the attributes from the function it's
calling on initialization.
LG, minor comments to address prior to the merge.
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
2521 | ||
2592 | ||
2620 | ||
2626 | ||
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
9786 | The parent function assumptions should always be in the assumed set, adding them here should not be necessary. | |
9813 | Also add the scope assumptions here, all of them are known. |
Consider passing the set as reference, unsure how much work copying it is