Page MenuHomePhabricator

[attrs] Handle convergent CallSites.
ClosedPublic

Authored by jlebar on Feb 29 2016, 1:55 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.

Originally landed as r261544, then reverted in r261544 for (incidental)
build breakage. Re-landed here with no changes.

Event Timeline

jlebar updated this revision to Diff 49422.Feb 29 2016, 1:55 PM
jlebar retitled this revision from to [attrs] Handle convergent CallSites..
jlebar updated this object.
jlebar added reviewers: chandlerc, jingyue.
jlebar added subscribers: hfinkel, jhen, tra, llvm-commits.

This should be entirely or mostly good-to-go, but would like a once-over by chanclerc, since the last attempt at landing this, D17317, didn't get his final approval.

jlebar removed a reviewer: jingyue.Feb 29 2016, 2:06 PM
jlebar added a subscriber: jingyue.

Chandler, friendly ping.

chandlerc accepted this revision.Mar 12 2016, 5:46 AM
chandlerc edited edge metadata.

Very nice. Two small nits below, feel free to land with those addressed.

lib/Transforms/IPO/FunctionAttrs.cpp
910–919 ↗(On Diff #49422)

Perhaps its just me, but I would find this easier to read as a single loop over the functions in the SCC, and three conditions on which we return false (with the exact comments you currently have, those are awesome).

923 ↗(On Diff #49422)

"if is CS a" -> "if CS is a"

This revision is now accepted and ready to land.Mar 12 2016, 5:46 AM
This revision was automatically updated to reflect the committed changes.
jlebar marked an inline comment as done.
jlebar added inline comments.Mar 14 2016, 4:07 PM
lib/Transforms/IPO/FunctionAttrs.cpp
910–919 ↗(On Diff #49422)

Hm, it's slightly more complicated than that, because the first 'if' is a !any_of. But I went ahead and made the change, because I think it's justified on the grounds of not wanting to iterate over a non-convergent function's instructions if it happens to be in an SCC with a convergent function.