[PATCH] D69498: IR: Invert convergent attribute handling

[Matt Arsenault via Phabricator via llvm-commits]

arsenm marked an inline comment as done.
arsenm added a comment.

In D69498#1723606 <https://reviews.llvm.org/D69498#1723606>, @rjmccall wrote:

> A note on spelling: the no prefix seems to be used largely with verbs; it's weird to use it here with an adjective, especially since noncovergent is just a letter away.


I don't think it reads any different to me than, say nofree->no free calls. noconvergent-> no convergent operations. nonconvergent sounds odder to me

> In D69498#1723553 <https://reviews.llvm.org/D69498#1723553>, @rjmccall wrote:
> 
>> This certainly seems like a more tractable representation, although I suppose it'd be a thorn in the side of (IR-level) outlining.
> 
> 
> Sorry, I mis-clicked and sent this review while I was still editing it.
> 
> I agree that this is a theoretically better representation for targets that care about convergence, since it biases compilation towards the more conservative assumption.  On the other hand, since the default language mode is apparently to assume non-convergence of user functions, and since the vast majority of targets do not include convergence as a meaningful concept, you're also just gratuitously breaking a ton of existing test cases, as well as adding compile time for manipulating this attribute.  Perhaps there should be a global metadata, or something in the increasingly-misnamed "data layout" string, which says that convergence is meaningful, and we should only add the attribute in appropriately-annotated modules?

The default language mode for the non-parallel languages. The set of languages not setting this does need to grow (SYCL and OpenMP device code are both currently broken).

I don't think there are other attributes with weird connections to some global construct. Part of the point of attributes is that they are function level context. I don't really understand the compile time concern; the cost of the attribute is one bit in a bitset. We have quite a few attributes already that will be inferred for a large percentage of functions anyway. The case that's really important to add in frontend is external declarations and other opaque cases, others will be inferred anyway. You could make the same argument with nounwind, since most languages don't have exceptions.



================
Comment at: clang/lib/CodeGen/CGObjCMac.cpp:4259
   call->setDoesNotThrow();
+  call->setNoConvergent();
   call->setCallingConv(CGF.getRuntimeCC());
----------------
rjmccall wrote:
> I don't think GPU thread convergence is a concept that could ever really be applied to a program containing ObjC exceptions, so while this seems correct, it also seems pointless.
It doesn't apply, so this needs to be explicitly marked as not having it. This bypasses the place that emits the language assumed default attribute so this is needed. A handful of tests do fail without this from control flow changes not happening


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69498/new/

https://reviews.llvm.org/D69498