[PATCH] D69498: IR: Invert convergent attribute handling

[Sameer Sahasrabuddhe via Phabricator via cfe-commits]

sameerds added a comment.

In D69498#2710675 <https://reviews.llvm.org/D69498#2710675>, @arsenm wrote:

> In D69498#2709218 <https://reviews.llvm.org/D69498#2709218>, @sameerds wrote:
>
>> 1. It is actually okay add control dependences to a convergent function as long as the new branches are uniform.
>> 2. Some times we hack a transform to avoid doing things that cannot be described as "add new control dependence", and still talk about the function having the above named attribute.
>>
>> There is another important bug that obfuscates the whole discussion: most places that use "isConvergent()" should in fact first check if it really matters to the surrounding control flow. There is too much loaded onto that one attribute, without sufficient context. The definition of "noconvergent" that I am proposing starts out by first fixing the definition of convergent itself. This definition is independent of target, and only talks about the properties of the control flow that reach the callsite. To repeat, this does not reinterpret the IR in a target-defined way. Like I said, in the new definition, all function calls are convergent even on CPUs, so I don't see where the target comes in. If you still insist on talking about interpretation of core attributes, please start by deconstructing the definition that I propose so I can see what I am missing.
>
> Yes, the current concept of convergent is broken. I think this whole recent burst of conversation has been too focused on the current convergent situation and this patch in particular. I think we need to conceptually rebase this into the world that D85603 <https://reviews.llvm.org/D85603> creates.

Actually that is exactly what this current conversation is doing. My definition of `noconvergent` first rebases the definition of `convergent` into what D85603 <https://reviews.llvm.org/D85603> does. My wording is no different than the one you see here: https://reviews.llvm.org/D85603#change-WRNH9XUSSoR8

> The expected convergence behavior is not only a target property, but of the source language. The frontend ultimately has to express the intended semantics of these cross lane regions, and the convergence tokens gives this ability.

That is true, and it is covered by the simple fact that we both agree that `convergent` needs to be the default unless specified by the frontend using `noconvergent`.

> My point about not making this target dependent is we shouldn't be trying to shoehorn in TTI checks around convergent operations just to save frontends from the inconvenience of adding another attribute to function declarations. We can interprocedurally infer noconvergent like any other attribute most of the time. The convergent calls and their tokens should be interpreted the same even if the target CPU doesn't really have cross lane behavior.

This is not an attempt to shoehorn TTI checks for mere convenience. You missed the part about how the current use of `CallInst::isConvergent()` is broken. This is where the wording in D85603 <https://reviews.llvm.org/D85603> inherits a major fault from the existing definition of convergent. It is wrong to say that optimizations are plain forbidden by the mere knowledge/assumption that some call is convergent. Those optimizations are forbidden only if divergent control flow is involved. The definition of `convergent` needs to refrain from being prescriptive about what the compiler can and cannot do. See the definition of `nosync` for comparison.

The convergent property merely says that the call is special, and then it is up to each optimization to decide what happens. That decision is based on whether the optimization affects divergent control flow incident on the call, and whether that effect is relevant to a convergent function. Now that's where the following reasoning comes into play:

1. Every use of isConvergent() needs to be tempered with knowledge of the control flow incident on it.
2. The only way to check for divergent control flow is to call on divergence analysis.
3. When divergence analysis is called on a target with no divergence, it returns `nullptr`, which is treated as equivalent to returning `uniform` for every query. This is not hypothetical, it's already happening whenever a transform does `if (!DA) { ... }`.
4. Pending an attempt to actually call DA at each of these places, a correct simplification is to assume uniform control flow when the target does not have divergence, and assume divergent control flow otherwise.

That's all there is to this. Like I keep saying, this is not a reinterpretation, nor is it a hack. From my first comment on the thread, it is an attempt to recast the whole question o be one about combining optimizations with target-specific information. Now before you balk at yet another use of the phrase "target-specific", please note that DA is totally target-specific. The "sources of divergence" must be identified by the target. So this is a perfectly legitimate place to say "target-specific". It's making the optimization "target-specific", not the attribute.


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

https://reviews.llvm.org/D69498