[PATCH] D69498: IR: Invert convergent attribute handling

sameerds added a comment. In D69498#2728451 , @JonChesterfield wrote: > I wonder if it is necessary for the exec mask to be implicit state, managed > by a convergent/divergent abstraction. > > We could reify it as an argument passed to kernels (where all

[PATCH] D69498: IR: Invert convergent attribute handling

JonChesterfield added a comment. I wonder if it is necessary for the exec mask to be implicit state, managed by a convergent/divergent abstraction. We could reify it as an argument passed to kernels (where all bits are set), and adjusted by phi nodes at entry to basic blocks. Various intrinsics

[PATCH] D69498: IR: Invert convergent attribute handling

sameerds added a comment. In D69498#2712706 , @nhaehnle wrote: > In D69498#2711396 , @foad wrote: > >> I don't have much to add to the conversation except to point out that this >> definition defines `noconvergent`

[PATCH] D69498: IR: Invert convergent attribute handling

nhaehnle added a comment. In D69498#2711396 , @foad wrote: > I don't have much to add to the conversation except to point out that this > definition defines `noconvergent` in terms of divergent control flow, but the > langref itself doesn't define what d

[PATCH] D69498: IR: Invert convergent attribute handling

foad added a comment. In D69498#2705441 , @sameerds wrote: > I would propose refining the definition of the `noconvergent` attribute as > follows: > >> noconvergent: >> >> Some targets with a parallel execution model provide cross-thread operations >> wh

[PATCH] D69498: IR: Invert convergent attribute handling

sameerds added a comment. In D69498#2710675 , @arsenm wrote: > In D69498#2709218 , @sameerds wrote: > >> 1. It is actually okay add control dependences to a convergent function as >> long as the new branches are un

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. In 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 > d

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. What matter isn't "convergent" in itself, it is how it restricts transformations: if you know that all the control flow is always uniform, are there still restriction on any transformation in presence of convergent instructions? If not, then the "target approach" s

[PATCH] D69498: IR: Invert convergent attribute handling

sameerds added a comment. In D69498#2708990 , @arsenm wrote: > You're still saying the same thing. This needs to be defined generically. > Frontends don't *have* to to anything, they'll just get the assumed > convergent behavior by default. Either fronte

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. In D69498#2707100 , @sameerds wrote: >>> 1. The meaning of the `convergent` attribute has always been >>> target-dependent. >>> 2. Dependence on TTI is not a real cost at all. We may eventually update >>> every use of isConvergent

[PATCH] D69498: IR: Invert convergent attribute handling

sameerds added a comment. In D69498#2706524 , @arsenm wrote: > In D69498#2705441 , @sameerds wrote: > >> I realize now that what @foad says above puts the idea in a clearer context. >> Essentially, any check for is

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. In D69498#2705441 , @sameerds wrote: > I realize now that what @foad says above puts the idea in a clearer context. > Essentially, any check for isConvergent() isn't happening in a vacuum. It is > relevant only in the presence of

[PATCH] D69498: IR: Invert convergent attribute handling

sameerds added a comment. I realize now that what @foad says above puts the idea in a clearer context. Essentially, any check for isConvergent() isn't happening in a vacuum. It is relevant only in the presence of divergent control flow, which in turn is relevant only when the target has diverge

[PATCH] D69498: IR: Invert convergent attribute handling

foad added a comment. > But in practice, the main issue for everyone is the effect on compile time > for targets that don't care about convergence/divergence. For such targets, > running even the divergence analysis is an unnecessary cost. LegacyDivergenceAnalysis::runOnFunction bails out immed

[PATCH] D69498: IR: Invert convergent attribute handling

sameerds added a comment. In D69498#2704364 , @foad wrote: >> This recasts the whole question to be one about combining optimizations with >> target-specific information. The only changes required are in transforms >> that check `CallInst::isConvergent()

[PATCH] D69498: IR: Invert convergent attribute handling

sameerds added a comment. The way I see it, the notion of convergence is relevant only to a certain class of targets (usually represented by GPUs) and it only affects certain optimizations. Then why not have only these optimizations check `TTI` to see if convergence matters? `TTI.hasBranchDiver

[PATCH] D69498: IR: Invert convergent attribute handling

foad added a comment. In D69498#2703317 , @sameerds wrote: > The way I see it, the notion of convergence is relevant only to a certain > class of targets (usually represented by GPUs) and it only affects certain > optimizations. Then why not have only th

[PATCH] D69498: IR: Invert convergent attribute handling

JonChesterfield added a subscriber: sameerds. JonChesterfield added a comment. Herald added subscribers: Naghasan, lxfind, okura, bbn, kuter, kerbowa, pengfei, jrtc27. Herald added a reviewer: sstefan1. Herald added a reviewer: baziotis. I've managed to run a bug in some freestanding c++ compiled

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. Talked with Matt at the last social, they thought I was strongly opposed to this: I am not, and I told them I would clarify here: I don't buy the argument of "frontend projects have consistently run into problems related to this", I think this is not a good justific

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1735763 , @arsenm wrote: > In D69498#1731265 , @mehdi_amini > wrote: > > > Just thought about a slight variation on this: what about adding a flag on > > the datalayout (or a

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. In D69498#1731265 , @mehdi_amini wrote: > In D69498#1727650 , @dexonsmith > wrote: > > > In D69498#1723606 , @rjmccall > > wrote: > > > > > Perhaps

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. In D69498#1731265 , @mehdi_amini wrote: > In D69498#1727650 , @dexonsmith > wrote: > > > In D69498#1723606 , @rjmccall > > wrote: > > > > > Perhap

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1727650 , @dexonsmith wrote: > In D69498#1723606 , @rjmccall wrote: > > > Perhaps there should be a global metadata, or something in the > > increasingly-misnamed "data layout

[PATCH] D69498: IR: Invert convergent attribute handling

Anastasia added a comment. In D69498#1725692 , @kariddi wrote: > In D69498#1725528 , @rjmccall wrote: > > > It absolutely makes sense for Clang as a GPU-programming frontend to set > > attributes appropriately when

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. As far as optimization inhibition is concerned, noconvergent will be inferred for all functions that don't call convergent intrinsics (i.e. the state of the world for all functions on all CPU targets). The frontend needing to do something for optimization comes up in rel

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. In D69498#1728039 , @tra wrote: > Perhaps we can deal with that by providing a way to specify per-module > default for the assumed convergence of the functions and then checking in the > back-end (only those that do care about con

[PATCH] D69498: IR: Invert convergent attribute handling

tra added a comment. Would I be too far off the mark to summarize the situation this way? - current default for unattributed functions is unsound for GPU back-ends, but is fine for most of the other back-ends. - it's easy to unintentionally end up using/mis-optimizing unattributed functions for

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a comment. In D69498#1727626 , @mehdi_amini wrote: > In D69498#1727546 , @jdoerfert wrote: > > > In D69498#1727419 , @mehdi_amini > > wrote: > > > > > In D69

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. As I've said before, I absolutely view this polarity flip as good for GPU compilers, and I don't mind Clang needing to take some patches to update how we operate when targeting the GPU and to update some GPU-specific tests. I do not think we should view GPU targets as

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. In D69498#1727626 , @mehdi_amini wrote: > In D69498#1727546 , @jdoerfert wrote: > > > In D69498#1727419 , @mehdi_amini > > wrote: > > > > > In D69498

[PATCH] D69498: IR: Invert convergent attribute handling

dexonsmith added a comment. In D69498#1723606 , @rjmccall wrote: > 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 attrib

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1727546 , @jdoerfert wrote: > In D69498#1727419 , @mehdi_amini > wrote: > > > In D69498#1727080 , @jdoerfert > > wrote: > > > > > Let

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a subscriber: hfinkel. jdoerfert added a comment. In D69498#1727419 , @mehdi_amini wrote: > In D69498#1727080 , @jdoerfert wrote: > > > Let me quote @arsenm here because this is so important: "Basica

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. In D69498#1727419 , @mehdi_amini wrote: > In D69498#1727080 , @jdoerfert wrote: > > > Let me quote @arsenm here because this is so important: "Basically no > > frontend has gotten this right

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1727080 , @jdoerfert wrote: > Let me quote @arsenm here because this is so important: "Basically no > frontend has gotten this right, including clang and non-clang frontends." For > me, that statement alone is reaso

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a comment. In D69498#1726610 , @mehdi_amini wrote: > > The short version is the fact that most developers aren't aware of and > > don't understand the subtleties of convergence, and making sure the > > front-end does something in all cont

[PATCH] D69498: IR: Invert convergent attribute handling

JonChesterfield added a comment. Requiring the presence of an attribute for correctness is a bad thing. OpenMP was missing this annotation in a number of places and is probably still missing it elsewhere. I wouldn't bet on handwritten bitcode libraries getting it right everywhere either. An opt

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. In D69498#1725867 , @jdoerfert wrote: > In D69498#1725819 , @mehdi_amini > wrote: > > > Maybe we can start by looking into the motivation for this patch: > > > > > There is a burden on

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. In D69498#1725819 , @mehdi_amini wrote: > Maybe we can start by looking into the motivation for this patch: > > > There is a burden on frontends in environments that care about convergent > > operations to add the attribute just in

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a comment. In D69498#1725819 , @mehdi_amini wrote: > Maybe we can start by looking into the motivation for this patch: > > > There is a burden on frontends in environments that care about convergent > > operations to add the attribute just

[PATCH] D69498: IR: Invert convergent attribute handling

mehdi_amini added a comment. Maybe we can start by looking into the motivation for this patch: > There is a burden on frontends in environments that care about convergent > operations to add the attribute just in case it is needed. This has proven to > be somewhat problematic, and different fro

[PATCH] D69498: IR: Invert convergent attribute handling

kariddi added a comment. In D69498#1725528 , @rjmccall wrote: > It absolutely makes sense for Clang as a GPU-programming frontend to set > attributes appropriately when targeting the GPU. I'm objecting to making > "convergence" and related "code layout

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. In D69498#1724625 , @kariddi wrote: > One thing to probably note is that its not only a "target specific" issue, > but a language specific issue as well (IMHO). OpenCL, CUDA, SYCL are all > languages (to name a few) that have a

[PATCH] D69498: IR: Invert convergent attribute handling

nhaehnle added a comment. As you know, I am very much in favor of this change, and really anything that re-establishes the rule that code is treated maximally conservatively when there are no attributes or metadata. There is one slight concern here because what we arguably need in SIMT targets

[PATCH] D69498: IR: Invert convergent attribute handling

kariddi added a comment. One thing to probably note is that its not only a "target specific" issue, but a language specific issue as well (IMHO). OpenCL, CUDA, SYCL are all languages (to name a few) that have a concept of "convergence" and its not related only to the fact that they mostly run o

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm added a comment. I think the question should be what the IR policy is for properties that are required for correctness and not necessarily what most users will use. There's a general trend towards functions being correct by default, and attributes adding optimization possibilities. conve

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a comment. @rjmccall Thanks for the quick response! I tried to describe my view below. To avoid confusion on my side, could you maybe describe what you think when/which attribute should be created, derived, and used during the optimization pipeline? In D69498#1724232

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. If you can come up with a way to make this change that doesn't require changes to non-GPU frontends or tests, I agree that treating functions as convergent by default makes sense for GPU targets. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69498/new/ https:

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. The kind of tracking that you're doing of convergent operations is one example of a basically limitless set of "taint" analyses, of which half a dozen already exist in LLVM. I object to the idea that an arbitrary number of unrelated tests need to be changed every time

[PATCH] D69498: IR: Invert convergent attribute handling

jdoerfert added a reviewer: JonChesterfield. jdoerfert added a comment. Before we get lost in review details let's make sure we agree that his is the right path. I mean, transition from `convergent` to `noconvergent` (or a similar spelling). I do support this by the way. In D69498#1723606

[PATCH] D69498: IR: Invert convergent attribute handling

arsenm marked an inline comment as done. arsenm added a comment. In 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

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. In 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 s

[PATCH] D69498: IR: Invert convergent attribute handling

rjmccall added a comment. This certainly seems like a more tractable representation, although I suppose it'd be a thorn in the side of (IR-level) outlining. 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 `no