Page MenuHomePhabricator

[FunctionAttrs][NPM] Fix handling of convergent
ClosedPublic

Authored by aeubanks on Oct 20 2020, 2:33 PM.

Details

Summary

The legacy pass didn't properly detect indirect calls.

We can still remove the convergent attribute when there are indirect
calls. The LangRef says:

When it appears on a call/invoke, the convergent attribute indicates

that we should treat the call as though we’re calling a convergent
function. This is particularly useful on indirect calls; without this we
may treat such calls as though the target is non-convergent.

So don't skip handling of convergent when there are unknown calls.

Diff Detail

Event Timeline

aeubanks created this revision.Oct 20 2020, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2020, 2:33 PM
aeubanks requested review of this revision.Oct 20 2020, 2:33 PM

Before I look at the changes in more detail, two high level comments, one we need to address actually. (And more people).

  1. I find this sentence troublesome: This is particularly useful on indirect calls; without this we may treat such calls as though the target is non-convergent. We should say we do treat them or we don't, but not we may. That said, I am unsure if treating indirect calls as non-convergent regardless of what the function is is a good idea. auto t = (&barrier); t(); seems to be the minimal example to make my point.
  2. I would encourage someone that cares about convergent (which in theory includes me but not *right now*) to add this deduction to the Attributor.

At a high level, it should be fine to assume an indirect call without the "convergent" attribute isn't convergent. If the language rules for some language say the callee of an indirect call might be convergent, the frontend can add the convergent attribute to that call. (Unlike most attributes, it's a negative attribute: it restricts optimizations, and attribute inference optimizations would remove it,)

At a high level, it should be fine to assume an indirect call without the "convergent" attribute isn't convergent. If the language rules for some language say the callee of an indirect call might be convergent, the frontend can add the convergent attribute to that call. (Unlike most attributes, it's a negative attribute: it restricts optimizations, and attribute inference optimizations would remove it,)

I don't object to that but I want us to remove the "may" from the lang ref and make it a "will" (or similar). It describes the semantics not the optimization we "may" perform.

At a high level, it should be fine to assume an indirect call without the "convergent" attribute isn't convergent. If the language rules for some language say the callee of an indirect call might be convergent, the frontend can add the convergent attribute to that call. (Unlike most attributes, it's a negative attribute: it restricts optimizations, and attribute inference optimizations would remove it,)

I don't object to that but I want us to remove the "may" from the lang ref and make it a "will" (or similar). It describes the semantics not the optimization we "may" perform.

I'd be fine with this change temporarily. Longer term, we should just do D85603.

I'd argue that making the change s/may/will/ does not actually change meaning here (because if a consumer of LLVM IR may treat calls this way, then any frontend must conservatively assume that it will), and it also doesn't change the fact that the LangRef talks about allowed optimizations instead of semantics. The latter is an inherent downside of the current definition of convergent, and one of the things that D85603 solves.

Oh, and on the change itself: I'm not familiar enough with the attributor framework to judge the implementation, but the described reasoning (being able to make deductions from indirect calls) is sound.

Oh, and on the change itself: I'm not familiar enough with the attributor framework to judge the implementation, but the described reasoning (being able to make deductions from indirect calls) is sound.

I might try to add this to attributor soon.

llvm/lib/Transforms/IPO/FunctionAttrs.cpp
1306

Nit: This doesn't remove convergent anymore.

aeubanks updated this revision to Diff 301696.Oct 29 2020, 11:20 AM

update comment

jdoerfert accepted this revision.Nov 23 2020, 1:35 PM

LGTM.

It took me a while to see that the de-duplication of the SCCNodeSet contained the change that was important.

This revision is now accepted and ready to land.Nov 23 2020, 1:35 PM
This revision was landed with ongoing or failed builds.Nov 23 2020, 9:10 PM
This revision was automatically updated to reflect the committed changes.