This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Attributor call site specific AAValueConstantRange
ClosedPublic

Authored by kuter on Jul 13 2020, 10:16 PM.

Details

Summary

This patch makes uses of the context bridges introduced in D83299 to make
AAValueConstantRange call site specific.

Diff Detail

Event Timeline

kuter created this revision.Jul 13 2020, 10:16 PM
Herald added a reviewer: sstefan1. · View Herald Transcript
Herald added a reviewer: uenoku. · View Herald Transcript
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
kuter retitled this revision from [Attributor] Attributor call base specific AAValueConstantRange to [Attributor] Attributor call site specific AAValueConstantRange.Jul 13 2020, 10:25 PM

Cool! As discussed, merge D83299 and add new call sites for test1.

kuter updated this revision to Diff 278199.Jul 15 2020, 8:25 AM

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 ?

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?

kuter added a comment.Jul 15 2020, 2:33 PM

Cool! As discussed, merge D83299 and add new call sites for test1.

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?

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.

uenoku added inline comments.Jul 15 2020, 7:09 PM
llvm/test/Transforms/Attributor/cb_range.ll
2 ↗(On Diff #278199)

Could you run for the case of --attributor-enable-call-site-specific=false as a comparison?

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.

I am not sure we can help you much without the actual failure output.

kuter updated this revision to Diff 278627.Jul 16 2020, 4:29 PM

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
2 ↗(On Diff #278199)

we should do this for the old and new PM, as well as the module and CGSCC pass.
so take the 4 run lines we have in other tests, duplicate them so you have them once w/ and once w/o the flag. The check prefixes might need to be adjusted in that case but basically you want check prefixes to overlap so if the code is the same it is shown in the check lines right away.

kuter updated this revision to Diff 279029.Jul 18 2020, 3:28 PM

fix typo.
improve code readability.

kuter marked 2 inline comments as done.Jul 18 2020, 3:29 PM

Some minor comments below.

Upload diffs with full context please.

I will start using arcanist so I don't make mistakes like this again.

kuter updated this revision to Diff 279034.Jul 18 2020, 4:23 PM
kuter edited the summary of this revision. (Show Details)

Add test combinations.

kuter marked 2 inline comments as done.Jul 18 2020, 4:23 PM
kuter updated this revision to Diff 279038.Jul 18 2020, 4:53 PM

fix run lines (there was a mistake in the earlier revision).

jdoerfert accepted this revision.Jul 18 2020, 6:07 PM

LGTM with some comments to be addressed. This is cool :)

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
550

Nit: an 'argument'

556

This should not happen, right? Make it an assert instead.

561

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).

621–627

Use cast<..> and keep it as reference.

This revision is now accepted and ready to land.Jul 18 2020, 6:07 PM
kuter updated this revision to Diff 279063.Jul 18 2020, 11:47 PM

small improvements.

There is a probelem with the tests since the last revision, will investigate.

kuter marked 4 inline comments as done.Jul 18 2020, 11:48 PM
fhahn added a subscriber: fhahn.Jul 19 2020, 3:03 AM
fhahn added inline comments.
llvm/test/Transforms/Attributor/cb_range.ll
164 ↗(On Diff #279063)

Shouldn't we also check what the range metadata actually contains?

kuter marked an inline comment as done.Jul 19 2020, 11:31 AM
kuter added inline comments.
llvm/test/Transforms/Attributor/cb_range.ll
164 ↗(On Diff #279063)

The positive and negative checks effectively check the upper and the lower bounds of the ranges

jdoerfert added inline comments.Jul 19 2020, 12:11 PM
llvm/test/Transforms/Attributor/cb_range.ll
164 ↗(On Diff #279063)

We can simply add the check lines manually at the end :)

kuter updated this revision to Diff 280702.Jul 25 2020, 4:53 PM

fix tests.

kuter updated this revision to Diff 281099.Jul 27 2020, 7:25 PM

bug fix.
add range contents as test.

kuter updated this revision to Diff 319875.Jan 28 2021, 8:10 AM

Rebase. Changes in the test run lines.

kuter updated this revision to Diff 327021.Feb 28 2021, 8:34 PM

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.

This revision was landed with ongoing or failed builds.Mar 10 2021, 2:20 PM
This revision was automatically updated to reflect the committed changes.
RKSimon added inline comments.
llvm/include/llvm/Transforms/IPO/Attributor.h
1138

@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)

kuter added inline comments.Mar 11 2021, 6:32 AM
llvm/include/llvm/Transforms/IPO/Attributor.h
1138

Thanks , I am aware of this. CMake decided to rebuild everything after I did a git pull.
I am waiting for it to compile right now.

I just assumed that enabling assertions also enabled the expansive checks, I am sorry.
If it still doesn't work after your fix. We can just remove this check for now.