This patch makes uses of the context bridges introduced in D83299 to make
AAValueConstantRange call site specific.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
merge with D83299
add positive and negative tests.
@jdoerfert I have a problem with this patch even though I auto generate tests, llvm-lit fails.
Is this a bug with the update_test_checks.py script ?
Is the output of the script expected? Do all of them fail? Did you maybe specify the wrong opt binary?
I inspected the output that opt generates and it makes sense.
also the script's output (included in the patch) also makes sense the.
I will double check everything but this is weird.
File check fails to match the first check line.
llvm/test/Transforms/Attributor/cb_range.ll | ||
---|---|---|
3 | Could you run for the case of --attributor-enable-call-site-specific=false as a comparison? |
add attributor-enable-call-site-specific=false as a test.
fix the run lines.
@sstefan1 thanks for the help it was because I forgot to add < %s :/
Some minor comments below.
Upload diffs with full context please.
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
543 | Can we split this in two statements for readability. Also use a C++ cast please. I think we can actually do cast on the AA and then getState() should give the right type. We might need to adjust the state wrapper return type though. Hope this makes some sense. | |
574 | Success | |
llvm/test/Transforms/Attributor/cb_range.ll | ||
3 | we should do this for the old and new PM, as well as the module and CGSCC pass. |
LGTM with some comments to be addressed. This is cool :)
llvm/lib/Transforms/IPO/AttributorAttributes.cpp | ||
---|---|---|
530 | Nit: an 'argument' | |
536 | This should not happen, right? Make it an assert instead. | |
541 | We don't want the value here, do we. We want const auto &AA = A.getAAFor<AAType>(QueryingAttribute, IRPosition::callsite_argument(*CBContext, ArgNo)); which might be better than the plain value result as it has a context instruction (and the attached attributes). | |
600–606 | Use cast<..> and keep it as reference. |
small improvements.
There is a probelem with the tests since the last revision, will investigate.
llvm/test/Transforms/Attributor/cb_range.ll | ||
---|---|---|
164 | Shouldn't we also check what the range metadata actually contains? |
llvm/test/Transforms/Attributor/cb_range.ll | ||
---|---|---|
164 | The positive and negative checks effectively check the upper and the lower bounds of the ranges |
llvm/test/Transforms/Attributor/cb_range.ll | ||
---|---|---|
164 | We can simply add the check lines manually at the end :) |
Split cb_range.ll into two files cb_range_enabled.ll and cb_range_disabled.ll.
This is needed because the combination of prefixes we use triggers a edge case with the
update script.
For more information see D95794 which tries to solve the issue (it doesn't work).
I don't think there is any non O(N!) to fix this edge case.
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
1138 ↗ | (On Diff #329771) | @kuter This is breaking some EXPENSIVE_CHECKS builds such as http://lab.llvm.org:8011/#/builders/16/builds/7624 AbstractAttribute isn't defined until later in the header - oddly my MSVC build is quite happy (after my fix at rGe74d6269259e28225a494ce98bd1762cdac1fe89) |
llvm/include/llvm/Transforms/IPO/Attributor.h | ||
---|---|---|
1138 ↗ | (On Diff #329771) | Thanks , I am aware of this. CMake decided to rebuild everything after I did a git pull. I just assumed that enabling assertions also enabled the expansive checks, I am sorry. |
Nit: an 'argument'