Hmm, those test changes were unexpected.

LGTM

LGTM

Sure, why not.

Mostly seems reasonable to me, but two questions inline.

LGTM

This needs a test. It should be possible to extract one from the bug reports?

I do prefer the explicit isDivergentUse!

LGTM

LGTM

LGTM. Followup for WQM/WWM sounds good to me as well.

Thanks for doing this. For the codegen quality question, I wonder if something like the following could be done:

Also fixes support for targets without i16.

Nice work! It's missing tests though -- I'm not sure if we want to print this additional information in the divergence printer, but definitely a test for the atomic optimizer pass is required.

Based on a quick check this should be safe for all graphics clients. LGTM.

LGTM

LGTM, though I believe the hyperlinks in :ref: need to be without the underscore (and other examples of :ref: use seem to corroborate that).

Just to clarify, how is selection of global_load_ubyte and friends going to work? I assume similar to today where the load returns an s32 value, but instruction selection does matching based on the MemOperand remembering the size?

One more thought:

LGTM

LGTM

Okay, the possibility of an AssertZext is an interesting point. So let me try the other way around: What would the MIR at this stage look like to enforce an and?

Okay thanks, I see the logic now.

In D64946#1593675, @rampitec wrote:

In D64946#1593334, @nhaehnle wrote:

How about the following simpler logic:

• if the PHI is used by any basic-block prologue instruction (except other PHIs), then insert the COPY at the top of the basic block
• otherwise, insert the COPY after the basic-block prologue

In this case COPY in the prologue also shall be marked as prologue instruction somehow.

My understanding is that this is mostly related to CWSR. The trap handler has to be able to "replay" the GWS instruction.

The AMDGPU changes seem fine to me overall.

Thanks! Could you please also add a test to Analysis/ValueTracking/memory-dereferenceable.ll?

In D64911#1591502, @arsenm wrote:

I still think we should be handling these on the IR level

How about the following simpler logic:

Have you checked that this actually fixes the reported CTS failure?

But then following this logic, I still think that by analogy with G_ZEXT, the operation of COPY from s1 into vcc should have the semantics of ignoring the high bits of the "s1 which is really an s32". Since there's nothing in the MIR test which guarantees that the incoming high bits of $sgpr0 are 0, the resulting code needs to have some form of masking.

That seems reasonable to me.

Add missing test changes

Add test case and remove the legalizer part.

I'm taking this over.

In D64726#1587457, @arsenm wrote:

In D64726#1587233, @nhaehnle wrote:

This seems incorrect, doesn't it? The truncation disappeared.... (e.g., what if $sgpr0 is 0x10)

My current understanding of G_TRUNC is it's a no-op, and supposed to always be legal. This is supposed to be the legalized MIR, so theoretically this was generated by something that knew the original argument was zeroext from i1

This seems incorrect, doesn't it? The truncation disappeared.... (e.g., what if $sgpr0 is 0x10)

Oh, is that because the new node causes the intrinsics to be lowered to G_FMINNUM etc.? Why doesn't this affect any other targets?

Why are you removing the testcases for the intrinsics?

Yeah, let's take this.

Would obviously be good to fix the underlying issue, but sure, this seems reasonable.

You should probably wait a bit in case somebody else wants to chime in, but this looks good to me.

Address review comments

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/1220

I'm currently looking into it.

One nit, apart from that LGTM.

LGTM

Thanks, LGTM

LGTM

LGTM

LGTM

LGTM

Would it be possible to make default operands overridable automatically iff they are at the end of the operand list? I.e., if you have a suffix of default operands, then those can be overridden?

Thanks for dealing with this. Matt's suggestion is reasonable to me, either way LGTM.

LGTM

LGTM

Properly test based on wavefront size

Add a test case

ping

I'd prefer the alternative fix at D63871, since it doesn't require RPOT.

This does seem useful, although the description is overly narrow (what does nosync on its own have to do with freeing memory?).

Oh... radv declares compute LDS as an LLVM global variable but doesn't use rtld yet, right? Sorry, I missed that.

Trivial nitpick, but essentially LGTM.

LGTM

LGTM

LGTM

Address review

Address review

Thanks.