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".

ping

ping^2

LGTM

Please make sure you give this a reasonable amount of testing, but it totally makes sense to me.

In D88774#2327009, @jiaolu wrote:

In D88774#2325404, @arsenm wrote:

Please work on removing this feature from the backend instead. It's long past time to stop handling this in the compiler

There are several shader debugger tools/situation dependent on this feature.
e.g. one line like this "s_andn2_b32 vcc_lo, vcc_lo, 0 ;000974: 8A6A806A"
1). we feed the disassembly count "0x000974" to the shader debugger tool to get this line of code execution ,register input and output.
2).Also there is some other shader dump tool,e.g. umr. or windbg which output a stream of hardware codes, we can use the hardware code to find matched part of disassembly lines.
3). we also have one shader replacement tool , to drop and replace part of the elf hardware code, if we have full disassembly line with counter and hardware code, it would be handy where to edit hardware code.

Hope it explains.

How about splitting this change into two parts? Limit this change to generating mixes of 64- and 32-bit moves, and then separately consider what to do about those moves of registers where subregisters are overwritten immediately.

Why does this require a commit upstream?

LGTM

Looks reasonable to me.

I agree with Matt here. You should be able to do experiments locally. Perhaps sinking should be disabled entirely, or perhaps sinking should be improved to take register liveness into account.

I don't understand how this would help with shader replacement. Can you please explain how you plan to use this?

LGTM

LGTM

ping

I'm not trying to fully solve the live range splitting problem greedy regalloc hits. I'm trying to eliminate the isBasicBlockPrologue concept that fastregalloc trips over when inserting spills at the beginning of the block

It's unclear to me what this is trying to achieve. If it is to prevent

bb:
  <-- reload inserted here during live range splitting
  $exec = S_OR_B64 $exec, %other
  ... rest of code ...

... then this change only replaces it by:

bb:
  <-- reload inserted here during live range splitting
  $exec = S_OR_B64_term $exec, %other
  // fallthrough

Hi Carl, what's the status of this?

In D87674#2290548, @madhur13490 wrote:

The revert also highlights the problem that we don't have representative test in llvm/test :(

In D87674#2290434, @madhur13490 wrote:

In D87674#2290419, @arsenm wrote:

In D87674#2290418, @madhur13490 wrote:

In D87674#2290396, @mareko wrote:

Yes, this commit is incorrect. It completely breaks code linking in Mesa OpenGL. s_waitcnt is required at the end of all global functions that return values.

Please revert. @nhaehnle

I don't understand why would it fail. This patch just moves s_waitcnt to the caller so they would be executed anyway. I think I am missing something. It would be helpful to root cause if we can isolate to a small test case.

Shader returns aren't real returns and the "caller" doesn't wait

I see. So how should this be implemented? May be we conditionalize this patch just for compute?