Page MenuHomePhabricator

[Attributor] Use AAValueSimplify to simplify returned values
ClosedPublic

Authored by jdoerfert on Jun 7 2021, 4:58 PM.

Details

Summary

We should use AAValueSimplify for all value simplification, however
there was some leftover logic that predates AAValueSimplify in
AAReturnedValues. This remove the AAReturnedValues part and provides a
replacement by making AAValueSimplifyReturned strong enough to handle
all previously covered cases. Further, this improve
AAValueSimplifyCallSiteReturned to handle returned arguments.

AAReturnedValues is now much easier and the collected returned
values/instructions are now from the associated function only, making it
much more sane. We also do not have the brittle logic anymore that looks
for unresolved calls. Instead, we use AAValueSimplify to handle
recursion.

Useful code has been split into helper functions, e.g., an Attributor
interface to get a simplified value.

Diff Detail

Event Timeline

jdoerfert created this revision.Jun 7 2021, 4:58 PM
jdoerfert requested review of this revision.Jun 7 2021, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 7 2021, 4:58 PM
Herald added a subscriber: bbn. · View Herald Transcript
ormris removed a subscriber: ormris.Jun 11 2021, 2:56 PM
kuter added a comment.Jun 23 2021, 1:12 PM

Some comments.

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

genericValueTraversal does not look at callbase returned values.
So the AAReturnedValues no longer contains transitively returned values. Wouldn't this affect some attributes ?

4402–4403

I think the code can be made more readable here.

llvm/test/Transforms/Attributor/nocapture-2.ll
222

We are getting less dereferenceablity. Isn't this weird ?

jdoerfert added inline comments.Jun 24 2021, 8:30 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
1030

This is a good thing, I think. From the commit message:

AAReturnedValues is now much easier and the collected returned
values/instructions are now from the associated function only, making it
much more sane. We also do not have the brittle logic anymore that looks
for unresolved calls. Instead, we use AAValueSimplify to handle
recursion.

If you use value simplification you still look through the call sites into callers, even better than before. If you use AAReturnedValues you will be restricted to the values in that function, I think this is better overall.

4402–4403

Will do :)

llvm/test/Transforms/Attributor/nocapture-2.ll
222

Some of the attributes are dependent on known information and therefore iteration order dependent. We cannot really do much about that without introducing extra cost. That means, sometimes we regress because we reach a fixpoint before some other information is marked as known.

With this patch we propagate the 4 into more places but not the 8, which is worth a FIXME but not necessarily something I'd address here.

271

^^^

325

^^^

jdoerfert marked 3 inline comments as done.

Rebase, address comments, add FIXME

kuter accepted this revision.Jun 28 2021, 10:17 AM
This revision is now accepted and ready to land.Jun 28 2021, 10:17 AM

LGTM. Sorry that this took a long time. this patch is big :)

This revision was landed with ongoing or failed builds.Sat, Jul 10, 10:33 AM
This revision was automatically updated to reflect the committed changes.