This is an archive of the discontinued LLVM Phabricator instance.

[BasicAA] Take attributes into account when requesting modref info for a call site
ClosedPublic

Authored by igor-laevsky on Feb 15 2017, 7:51 AM.

Details

Summary

When requesting modref information between call site and memory location we will bailout with MRI_ModRef result if one of the nocapture, byval or operand bundle attributes aliases target location. However we can improve this decision by taking into account operand attributes. If for example we know that aliasing operand is readonly we should only infer MRI_Ref.

This is important for targets with heavy use of the deopt operand bundles since they imply readonly attribute for their pointers.

@davide, after this change llvm successfully compiles NewGVN/readattrs.ll test case. Overall this looks like a desired effect but I'm not completely certain. Can you take a look please?

Diff Detail

Event Timeline

igor-laevsky created this revision.Feb 15 2017, 7:51 AM
igor-laevsky edited the summary of this revision. (Show Details)Feb 15 2017, 8:01 AM
igor-laevsky added reviewers: hfinkel, sanjoy, davide.
igor-laevsky added subscribers: davide, llvm-commits.
dberlin added inline comments.
lib/Analysis/BasicAliasAnalysis.cpp
884

Uh?

davide added inline comments.Feb 17 2017, 3:18 PM
test/Transforms/NewGVN/readattrs.ll
1–3

The fact this test passes now is actually good, but I'd like to understand why it was failing before and now it's not. Can you please elaborate?

davide requested changes to this revision.Feb 17 2017, 3:18 PM
This revision now requires changes to proceed.Feb 17 2017, 3:18 PM
igor-laevsky added inline comments.Feb 20 2017, 9:09 AM
lib/Analysis/BasicAliasAnalysis.cpp
884

Idea here is to intersect smarts from AAResultBase with the result we've received in this function. I'm actually not certain if this is needed since "AAResults" does intersect results from all alias analyses. Plus AAResultBase currently doesn't have any smarts. So, yes, I'm not sure if we need this operation.

test/Transforms/NewGVN/readattrs.ll
1–3

This test passes local non-escaping object as a readonly nocapture function argument. Before my change alias analysis would have ignored readonly argument and concluded that call modifies "%a". I suspect that old GVN might use some internal smarts to determine that "%a" is unmodified by the call, while NewGVN relies on the alias analysis.

I see what is happening.
Memdep gets a modref answer from AA, so it uses callCapturesBefore and then uses that instead.

I'm fine with having AA do this in modref, or using callCapturesBefore in MemorySSA.

Given that the related code says:
"/ FIXME: this is really just shoring-up a deficiency in alias analysis.
/ BasicAA isn't willing to spend linear time determining whether an alloca
/ was captured before or after this particular call, while we are. However,
/ with a smarter AA in place, this test is just wasting compile time."

we may just want to go ahead with basicaa ;)

dberlin added inline comments.Feb 20 2017, 9:46 AM
lib/Analysis/BasicAliasAnalysis.cpp
784

Can you explain why you need static casts here and below?

igor-laevsky added inline comments.Feb 20 2017, 10:20 AM
lib/Analysis/BasicAliasAnalysis.cpp
784

This is to be in sync with similar places in this file. I don't really have a preference on how to convert this values.

reames added a subscriber: reames.Feb 24 2017, 5:22 PM

This change looks entirely correct, but I think it could simplify greatly by removing the merging logic. Unless that's needed (test case required), we should simplify.

... actually, after think a bit longer, I'm pretty sure the merging is required for the DSE case. I'll let you find the test case and explanation to justify why. :)

lib/Analysis/BasicAliasAnalysis.cpp
747

Given we don't refine this (after suggested change described below) outside of the block we should sink this into the if block.

768

It looks like most of the merging here is only needed for the fall through logic? If we remove that, doesn't this simplify greatly.

884

I believe this can be removed. Or at least, if it can't, it definitely warrants a patch of it's own. :)

igor-laevsky edited edge metadata.

I had removed confusing merging logic. It was there only to assist with possible improvements of the mod ref info during execution of the 'getModRefInfo' function. But since there are no known examples of when this is valuable, adding extra complexity doesn't worth it.

reames accepted this revision.Feb 28 2017, 8:57 AM

LGTM

This revision was automatically updated to reflect the committed changes.