Deduce "align" attribute in attributor.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
@jdoerfert
I know align attribute is integer attribute and we need somewhat different deduction with other attributes. However, I implement AAAlignImpl as a subclass of BooleanState for now. Is it better to add some base class for the integer attribute?
llvm/test/Transforms/FunctionAttrs/align.ll | ||
---|---|---|
35 ↗ | (On Diff #207864) | I want to test this for if (ICS && ICS.hasRetAttr(Attribute::Alignment)) return CompareOrSet(ICS.getRetAlignment()); but it does not work well now. |
Why not make it derive from IntegerState and use the integer as the alignment, zero would be the worst possible = unknown.
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
689 ↗ | (On Diff #207864) | Please add getters for the assumed and known alignment here. |
llvm/lib/Transforms/IPO/Attributor.cpp | ||
816 ↗ | (On Diff #207864) | Can you unify the two initialize methods in the base class (AAAlignImpl) with the index trick used in addIfNotExistent? |
840 ↗ | (On Diff #207864) | What you want to do use to take the minimum of all align values and the current state (IntegerState). Remember the state at the beginning of this function to determine if it changed. We also need a test for this. |
852 ↗ | (On Diff #207864) | For the return value you should do two things (for now):
Later, 1) would be replaced by a new ValueTracking.h getPointerAlignment method that does traversal, e.g., look through casts, compute new alignment based on constant geps, etc. You can work on the later points as well if you want. |
872 ↗ | (On Diff #207864) | There is (probably) no need for the ExistsAssume trick here, you can leave it but add a comment to explain that it is a shortcut to identify an optimistic fixpoint. |
llvm/test/Transforms/FunctionAttrs/align.ll | ||
22 ↗ | (On Diff #207864) | We want align 4 here. |
llvm/lib/Transforms/IPO/Attributor.cpp | ||
---|---|---|
257 ↗ | (On Diff #208102) | I think you want to return -1 so it is clearly not a valid argument position. |
772 ↗ | (On Diff #208102) | return value or argument, right? |
776 ↗ | (On Diff #208102) | One could use the existing attribute as "known" and not indicate a fixpoint so that we can compute a better value. |
786 ↗ | (On Diff #208102) | Why the if? Can't you take the minimum of the knownAlign and the maximum allowed? Or initialize the assumed to the maximum allowed value already in the initialization? |
787 ↗ | (On Diff #208102) | style: remove braces around single statements. |
838 ↗ | (On Diff #208102) |
"If no assumed state was used for reasoning, an optimistic fixpoint is reached earlier." |
856 ↗ | (On Diff #208102) | I think you want to first look at the in-flight AAAlign attribute before you look at the IR. |
llvm/test/Transforms/FunctionAttrs/align.ll | ||
42 ↗ | (On Diff #208102) | Can we have SCC tests as well |
llvm/lib/Transforms/IPO/Attributor.cpp | ||
---|---|---|
1977 ↗ | (On Diff #211567) | Are the statistics missing? |
1981 ↗ | (On Diff #211567) | Make the 1U << 29 a static const member with a proper name and description. |
1985 ↗ | (On Diff #211567) | The state is not initialized here as above. Call the general constructor from the specialized one (above) to avoid such inconsistencies. |
2023 ↗ | (On Diff #211567) | I think we should, for consistency, call getAssumedAlign() here. |
2376 ↗ | (On Diff #211567) | remove isPointerTy, see above. |
llvm/test/Transforms/FunctionAttrs/align.ll | ||
75 ↗ | (On Diff #211567) | check the call sites. |
43 ↗ | (On Diff #208608) | What happened to test5 again? Why is it commented out? |
57 ↗ | (On Diff #208608) | Please add a more meaningful SCC test, 3 functions, conditionals, malloc, etc. This one is a no-return recursion for which we can deduce everything. |