Invert the logic of the patch: mem intrinsics shouldn't be unconditionally marked nosync in the first place.

Fired too quickly, the Attributor actually had a bug and an incorrect lit test. This version should fix that.

Split up the patch as per @reames' request

In D101701#2750507, @nhaehnle wrote:

In D101701#2747864, @jdoerfert wrote:

nofree functions imply nofree arguments so it very much makes a difference if we make it harder to infer
and annotate the former. As we already moved to a more semantic-based definition of escaped we can actually
look at an example where this makes a difference. Take pointers passed via memory to see how you cannot just
move the nofree to account for the change made by this patch:

struct { int *a } s;
s.a = malloc()               // this is a store.
nofree_nocapture_only(&s)    // we can potentially show `s.a` is not captured in the function but we cannot
                             // annotate that fact as a `nofree` argument without argument promotion, which
                             // we cannot always do.
// s.a is still known to be dereferenceable even without `nosync`. The function `nofree` is needed, `nofree`
// argument attributes are not involved.

Is that actually true? Regardless of the nocapture flag, s.a may be passed to another thread which then frees it. (nocapture only applies to &s in the first place, and even then, it does not say anything about temporary copies of the pointer that may be passed to other threads.)

Hi Johannes,

In D101701#2744727, @jdoerfert wrote:

Let me put down some thoughts:

1. What is the motivation here? I mean, which functions, except very few intrinsic, would be "strong" nofree but not nosync?

Address review feedback: Adding the required change to FunctionAttrs that @reames pointed out; in the new version, I also made sure to really go through all test cases.

I tried my hands at a change in the opposite direction in D101701

Apologies if this is obvious somehow, but could you please point at those "deref" changes? I think I missed that part of the conversation. My hope would be that it could be treated orthogonally to the nofree/nosync question.

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 divergent control flow is, which makes it an incomplete spec. (Perhaps I'm just restating @arsenm's objections.) This seems unsatisfactory to me but I have no idea what to do about it. I agree with @sameerds that the current definition of convergent is too restrictive because in practice we really do want to be able to move convergent calls past uniform control flow.

I think the crux of the discussion is the second half of this sentence:

Thank you for doing this.

To address this there was an attempt to invert the behavior of convergent attribute in this patch (https://reviews.llvm.org/D69498) then the frontend wouldn't need to generate the attribute everywhere and the optimizer wouldn't need to undo what frontend does. The change in this review doesn't address (2) as far as I can see - it seems it only generalized old convergent semantics to cover the cases with non-uniform CF. I am not clear yet about the details of how and what frontend should generate in IR for this new logic but it looks more complex than before. And if we have to stick to the conservative approach of assuming everything is convergent as it is now this might complicate and slow down the parsing. So I am just checking whether addressing (2) is still feasible with the new approach or it is not a direction we can/should go?

Hi @jholewinski, sorry for missing your comment earlier. It's been a while! I still need to work through the rest of the comments here, but there's a pretty crucial point here that seems to have been missed:

Hi @Anastasia, thank you for your comments. I replied inline, but a high-level point upfront is that in many ways, this patch only exists because HLL don't really have well-defined semantics for convergent operations yet. Most of us have a shared mental model of what they should be for most high-level constructs, but intuition breaks down for the trickier corner cases. I made some proposals in the Khronos Memory Model TSG for how useful semantics could be added to HLL, taking the corner cases into account, but on my end all of this is on hold while I'm on leave.

The change makes sense to me. Does this need a documentation update?

In D99507#2667537, @hliao wrote:

In D99507#2665302, @nhaehnle wrote:

I think this requires a lot more thought.

+1

What I'd like to know: why are we reloading a lane mask via V_READFIRSTLANE in the first place? I would expect one of two types of reload:

1. Load from a fixed lane of a VGPR using V_READLANE.

That depends on how we spill a SGPR by writing a fixed lane or write an active lane. The 1st one, without saving/restoring, we will overwrite the live values in the inactive lanes. HPC workloads are hit by that issue and cannot run correctly.

I think this requires a lot more thought.

In D93376#2471740, @RalfJung wrote:

It's fair to say that this patch is a change of semantics by forbidding the use of the lifetime intrinsics in certain places where they were previously allowed.

They were allowed by the LangRef but not supported in the code

Also, I did not suggest to change the semantics without an RFC and everything else that is needed. You suggest otherwise in your response. I did propose alternatives to fix your bug, but most of my responses show how the restrictions you want to put in place are making (potentially existing) optimizations invalid.

The thing is, in our view (aqjune, nlopes, and me) you *are* changing the semantics by allowing the lifetime intrinsics to be used on things like "malloc". From all we can tell, the intrinsics were never meant to be used with anything but alloca, so for all intents and purposes, currently, they only support alloca.

Thanks for expanding on this documentation.

Superseded by D92924, D92925, D92926

Remove HandleWrapperFor which got de facto superseded by SsaContext
(subsequent patch). Update comment slightly to match.

Seems trivial enough, and there doesn't really seem to be an owner of this code...

v9:

• rebase on factored-out Handle infrastructure

v9:

• rebase on factored-out Handle infrastructure

(comparison being a total ordering)
/// This avoids having to add asserts the comparison operators that the states
/// are valid and users can test for validity of the cost explicitly.

Counterpoint: The asserts cost nothing in a release build, and for software maintainability, having the asserts once in the comparison operator is better than having them at every callsite. Users of this class will forget to put them there.

After our internal discussion of this fizzled out, I am no longer actually sure whether this analysis is correct.

It seems the MTYPE numbering was changed on gfx9, reflect that in the comment.

In D85603#2378229, @sameerds wrote:

In D85603#2361168, @jlebar wrote:

• Will this paint us into a corner wrt CUDA, and specifically sm70+?

/me summons @wash, who is probably a better person to speak to this than me.

My understanding is that the semantics of <sm70 convergent are pretty similar to what is described in these examples. But starting in sm70+, each sync operation takes an arg specifying which threads in the warp participate in the instruction.

I admit I do not fully understand what the purpose of this is. At one point in time I thought it was to let humans write (or compilers generate) code like this, where the identity of the convergent instruction does not matter.

// Warning, does not seem to work on sm75
if (cond)
  __syncwarp(FULL_MASK);
else
  __syncwarp(FULL_MASK);

but my testcase, https://gist.github.com/50d1b5fedc926c879a64436229c1cc05, dies with an illegal-instruction error (715) when I make cond have different values within the warp. So, guess not?

Anyway, clearly I don't fully understand the sm70+ convergence semantics. I'd ideally like someone from nvidia (hi, @wash) to speak to whether we can represent their convergent instruction semantics using this proposal. Then we should also double-check that clang can in fact generate the relevant LLVM IR.

To extrapolate from Vinod's answer, I would say that we can represent sm70+ convergence semantics with this proposal. The situation seems to be covered by the examples in the section on hoisting and sinking. Consider the following example copied from the spec:

define void @example(...) convergent {
  %entry = call token @llvm.experimental.convergence.entry()
  %data = ...
  %id = ...
  if (condition) {
    %shuffled = call i32 @subgroupShuffle(i32 %data, i32 %id) [ "convergencectrl"(token %entry) ]
    ...
  }
}

Here, hoisting subgroupShuffle() is generally disallowed because it depends on the identity of active threads. A CUDA builtin with a mask argument similarly identifies specific threads that must be active at the set of textually unaligned calls that synchronize with each other. So any change in the control flow surrounding those calls is generally disallowed without more information. The new representation doesn't seem to restrict a more informed optimizer that can predict how the threads evolve.

This makes sense to me.

LGTM

This seems reasonable to me.

LGTM modulo the inline comment.

Does this mean it's time to remove the lo-hi syntax altogether?

I'm going to follow up with another RFC about this on llvm-dev.

In D85603#2361168, @jlebar wrote:

I'm going to try to give feedback, but with the caveat that there's a huge amount of discussion here, and with my apologies that I can't read the whole thread's worth of context. It's a lot. Sorry that I'm probably bringing up things that have already been discussed.

Address the comments from @jlebar that I indicate I'd address,
except for changes affecting the Verifier -- I'll do those later.

Overall, I think the policy change proposed here isn't entirely unreasonable, but I do think it needs to be treated as a change of policy. Not everybody is necessarily aware of a 4 year old email thread; heck, I've been working on upstream LLVM and frontends for 5 years and I wasn't aware of this supposed policy where in reality, things weren't actually done according to what's written in the policy documents. If I don't know this after 5 years, how can anybody joining in the last 4 years possibly be aware other than through mere chance. What's written in the documents needs to matter, if only as part of making LLVM a welcoming and inclusive community. Hidden tribal knowledge is the opposite of those ideals.

ping^2

I replied on llvm-dev.

Considering that you are doing this in response to D83088, I think we have pre-existing evidence that this isn't workable as-is. There has to be some obligation on the people "thinking there should be more review" to actually engage in review. In particular, if they have previously demonstrated that they cannot be relied on for this, then the rule should not apply.

Hi Mehdi, this is not an appropriate place for this discussion. Yes, we have a general rule that patches can be reverted if they're obviously broken (e.g. build bot problems) or clearly violate some other standard. This is a good rule, but it doesn't apply here. If you think it does, please state your case in the email thread that I've started on llvm-dev for this very purpose. Just one thing:

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.

In D89826#2347822, @jdoerfert wrote:

In D89826#2346511, @efriedma wrote:

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.

If we're nitpicking over such details, I would point out that it would be better to structure the whole thing differently, with a single legalizer class in which the insertAcquire etc. methods have different paths based on the hardware generation.

LGTM

LGTM

LGTM

I noticed what I believe is one more correctness issues, plus a few assorted comments.

David, I don't think this is appropriate here. Let's take the discussion to llvm-dev.

In D89610#2339215, @arsenm wrote:

In D89610#2339205, @nhaehnle wrote:

Adding writelane absolutely makes sense to me, but:

Additionally, since we emit the final encoded instructions, this wasn't really matching readlane either.

:(

We have a bunch of places where we _always_ use real instructions, and presumably those are fine. But if we let real instructions creep into places where we usually use pseudos, we end up making the backend less efficient because over time, everybody will have to check for pseudos and reals (like in this particular case here). Can we instead add some sort of check that those real instructions aren't used during CodeGen? (The MI verifier should be able to check for this, for example.)

There is some reason we use the encoded versions for readlane/writelane that I never remember what it is

LGTM modulo that inline question.

Thanks, LGTM

This is fine for graphics.

Adding writelane absolutely makes sense to me, but:

Duplicate of D89596?

That was very long ago. My best guess would be "oversight".