This is an archive of the discontinued LLVM Phabricator instance.

[CFLAA] Tag actual arguments as AttrEscaped instead of AttrUnknown
ClosedPublic

Authored by grievejia on Jun 11 2016, 3:34 PM.

Details

Summary

The primary focus of this patch is to mark actual arguments AttrEscaped and mark everything they point to AttrUnknown. This allows us to get better precision when an alias query contains at least one value that escapes by being an argument at an opaque callsite.

It also did a bit of refactoring for the graph building codes. I probably need to apologize, since I should have separate the refactoring and the functionality changes into two different patches.

Diff Detail

Event Timeline

grievejia updated this revision to Diff 60449.Jun 11 2016, 3:34 PM
grievejia retitled this revision from to [CFLAA] Tag actual arguments as AttrEscaped instead of AttrUnknown.
grievejia updated this object.
grievejia added a subscriber: llvm-commits.
grievejia updated this revision to Diff 60452.Jun 11 2016, 6:44 PM

Separate escaped values from external values

Yay precision!

I should have separate the refactoring and the functionality changes into two different patches

So... why wasn't this done, then?

lib/Analysis/CFLAliasAnalysis.cpp
132

Redundant private

158

Looks like all the users of this fail if we return null. Is there a reason we try to handle it gracefully here?

164

Same here.

180

FWIW, I had to remove these ctors from D21233, because they made the windows buildbots angry. Apparently it's a bug in MSVC2013.

If these don't have any use, please remove them from this patch, as well.

237

Style nit: Braces.

371

Comment disappeared?

372

Is there a reason we went from /// to // for these?

483

const auto &

502

Useless braces

504

Please fold this into the above condition

511

Useless braces

520

Please remove the braces here and below...

(And the parens on !CS.onlyReadsMemory())

520

Why do we care about the size of Targets here? Looks like we'll mark nothing as escaped if we can't find a target of a function.

526

auto *

527

Looks like we won't add this to the sets if Fn->doesNotAlias(0).

660

Is there a reason we can't hand back a const& here instead?

Also, same thoughts about things that look+feel like accessors (but call move) as in D21261.

663

And here

824

This seems pointless to have as a lambda if we're just using it in one place.

834

Same for this.

840

If we hand back a const& above, this needs to become a const auto&. Also, why not just call GraphBuilder.getExternalValues() in the range for?

841

auto *

844

This, too.

845

auto *

test/Analysis/CFLAliasAnalysis/attr-escape.ll
53

Can we have a test for callsites marked as readonly, (if we currently support them) as well?

I should have separate the refactoring and the functionality changes into two different patches

So... why wasn't this done, then?

By the time I realized this, I've already made too many modifications...

Also, let me rebase this patch to get rid of the changes that have already been applied to the patch that this one depends on (apparently there doesn't seem to have "phabricator rebase" functionality...).

grievejia marked 2 inline comments as done.Jun 13 2016, 2:06 PM

Also, let me rebase this patch to get rid of the changes that have already been applied to the patch that this one depends on (apparently there doesn't seem to have "phabricator rebase" functionality...).

Sorry, I meant "to include the changes that have already been...".

lib/Analysis/CFLAliasAnalysis.cpp
158

IMO deferring the null-check to the users is more flexible. If we hardcode the check inside this function, things could become ugly if we add another user that wants the result of the lookup but doesn't want to panic if the lookup fails.

We only have two callers here, so I guess the typing overhead is still acceptable?

180

This will be fixed by the rebase.

237

What exactly is the problem here? Should I remove the braces after "if (auto CExpr = ...", or should I add braces after "if (hasUsefulEdges..."?

371

Ooops...

372

The struct becomes private. Only tryInterproceduralAnalysis() uses it now.

520

I wasn't quite sure whether to use CS.onlyReadsMemory() or CS.getCalledFunction()->onlyReadsMemory() here. It looks like the two functions essentially do the same thing. Initially used the latter and switched to the former after a while (the size check was there just to make sure CS.getCalledFunction() != nullptr). So I think I'll just remove it now.

527

Which is intended: noalias return values are treated like mallocs.

test/Analysis/CFLAliasAnalysis/attr-escape.ll
53

I'm not aware that we can mark the callsite (instead of the function) readonly. How can we distinguish the two in our codes?

grievejia updated this revision to Diff 60610.Jun 13 2016, 2:22 PM
grievejia edited edge metadata.
grievejia marked 18 inline comments as done.

Rebase and style updates.

lib/Analysis/CFLAliasAnalysis.cpp
132

Not done

158–213

If we see the flexibility being useful in the future, then I'm fine with it; I was just wondering if this was intentional or not.

180

Looks like they might have slipped by. :)

237

The former, please.

Generally, the style rule is "useless braces shouldn't exist, unless they help readability somehow."

372

I don't see what that has to do with the comment style, but I agree that it probably doesn't matter.

527

Ah, I forgot that we added the nodes at the beginning of the function now -- my mistake. :)

533

Sounds good.

(Also, it looks like CS.onlyReadsMemory() will check for callsite attributes (e.g. readonly/readnone) for us, so you picked the better option there, I think.)

test/Analysis/CFLAliasAnalysis/attr-escape.ll
53

If you're talking about tests, call void @foo(args) readonly is the syntax, IIRC. It's definitely less common than just using a function-level attribute.

If you're talking about in CFLAA, I think I sort of addressed that in an earlier response.

grievejia updated this revision to Diff 60641.Jun 13 2016, 7:31 PM
grievejia marked 3 inline comments as done.

More style fixes and an updated test.

george.burgess.iv edited edge metadata.

LGTM. Thanks for the patch!

This revision is now accepted and ready to land.Jun 14 2016, 11:17 AM
This revision was automatically updated to reflect the committed changes.
grievejia added inline comments.Jun 20 2016, 3:45 PM
lib/Analysis/CFLAliasAnalysis.cpp
372

It's not exposed to the outside world so I assume we don't need to use Doxygen style comment?