This is an archive of the discontinued LLVM Phabricator instance.

[attrs] Handle convergent CallSites.
ClosedPublic

Authored by jlebar on Feb 16 2016, 5:02 PM.

Details

Summary

Previously we had a notion of convergent functions but not of convergent
calls. This is insufficient to correctly analyze calls where the target
is unknown, e.g. indirect calls.

Now a call is convergent if it targets a known-convergent function, or
if it's explicitly marked as convergent. As usual, we can remove
convergent where we can prove that no convergent operations are
performed in the call.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 48131.Feb 16 2016, 5:02 PM
jlebar retitled this revision from to [attrs] Handle convergent CallSites..
jlebar updated this object.
jlebar added reviewers: chandlerc, jingyue.
jlebar added subscribers: llvm-commits, tra, jhen, hfinkel.
chandlerc edited edge metadata.Feb 16 2016, 5:08 PM

See my comment below, but it seems like the removal of the call instruction attribute makes more sense in instcombine or something like it when we see a direct call to a non-convergent function (we don't need any call graph analysis for that). Then this code would just need to handle removing the attribute from function definitions when we can do a conservative analysis of an entire SCC and determine that no convergent operation is actually reached.

These should still iterate nicely in a bottom-up fashion.

lib/Transforms/IPO/FunctionAttrs.cpp
938–939 ↗(On Diff #48131)

The first claim doesn't seem true. This code bails when the SCC contains a call to a declaration, and we don't even call this routine if the SCC contains an external call.

jlebar updated this revision to Diff 48142.Feb 16 2016, 7:10 PM
jlebar marked an inline comment as done.
jlebar edited edge metadata.

Update per chandlerc's review.

This is a lot better, thank you. I went ahead and split up the test.

chandlerc added inline comments.Feb 16 2016, 7:35 PM
lib/Transforms/IPO/FunctionAttrs.cpp
952–958 ↗(On Diff #48142)

When these become multi-line lambdas with names and potential for confusing double negation, I have to say I find the early exit from a range based for loop substantially easier to read...

lib/Transforms/InstCombine/InstCombineCalls.cpp
2039–2042 ↗(On Diff #48142)

It's not clear that it is correct to fall through here... Usually these things go back onto a worklist and get visited again anyways. Is there a reason you chose this particular pattern?

jlebar updated this revision to Diff 48147.Feb 16 2016, 8:34 PM
jlebar marked 2 inline comments as done.

Update per Chandler's review.

lib/Transforms/InstCombine/InstCombineCalls.cpp
2039–2042 ↗(On Diff #48142)

No particular reason; I was following the Indices.empty() branch above, which seems to be doing something similar. I changed it as suggested.

jingyue added inline comments.Feb 16 2016, 10:02 PM
lib/Transforms/IPO/FunctionAttrs.cpp
955–958 ↗(On Diff #48147)

Maybe this reads better

if (CS && CS.isConvergent()) {
  if (SCCNodes.count(CS.getCalledFunction()) == 0)
    return false;
}

This reads exactly like your comment -- if CS is a convergent call to a function not in SCC.

958 ↗(On Diff #48147)

What happens if CS.getCalledFunction() == nullptr? i.e. CS calls a function pointer.

jlebar updated this revision to Diff 48156.Feb 16 2016, 11:17 PM

Combine if statements in removeConvergentAttrs.

jlebar marked 2 inline comments as done.Feb 16 2016, 11:18 PM
jlebar added inline comments.
lib/Transforms/IPO/FunctionAttrs.cpp
955–958 ↗(On Diff #48147)

Maybe this reads better

Sure, I think that's better; combined the if statements.

What happens if CS.getCalledFunction() == nullptr?

We do a lookup of nullptr in the map, which is fine. (This is covered by a testcase -- I hope. :)

jlebar marked an inline comment as done.Feb 19 2016, 4:01 PM

Friendly ping -- are we happy with this?

jingyue accepted this revision.Feb 20 2016, 9:06 PM
jingyue edited edge metadata.

Happy!

This revision is now accepted and ready to land.Feb 20 2016, 9:06 PM

Thanks. Will wait for chandlerc, since he has opinions.

This revision was automatically updated to reflect the committed changes.

So much for waiting; I accidentally committed this because I fail at git.

On the bright side, it has a strange test failure that manifested only after sync'ing, so I get to revert.

Reverted in r261549.

jlebar added a comment.EditedFeb 22 2016, 11:18 AM

Wrong link: New review in D17518.

jlebar added a comment.EditedFeb 22 2016, 11:31 AM

New review in D17518.

Sorry, posted in wrong tab.

The relevant link here is https://gist.github.com/57285538454829420e0e -- with this change (which fixes a bug where we'd sometimes return MadeChange = true when we should have returned MadeChange = false), the readattrs.ll test fails.

$ opt < /usr/local/google/home/jlebar/code/llvm/src/test/Transforms/FunctionAttrs/readattrs.ll -aa-pipeline=basic-aa -passes='require<targetlibinfo>,cgscc(function-attrs)' -S -debug
https://gist.github.com/0c003a39199904e230dc

OK, now that things are working again, new review in D17739. Would appreciate a review there, Chandler, to make sure you're happy with this patch.