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.

In D63420#1545977, @arsenm wrote:

In D63420#1545917, @nhaehnle wrote:

What does this actually fix?

This is likely to pessimize codegen for graphics quite badly. Graphics APIs make fairly strong aliasing guarantees which we don't properly express in LLVM at the moment, and we kind of get by without it by having s.buffer.load be readnone.

When trying to do some global isel work, I ran into inconsistencies in the attributes (e.g. D63422), and I don't want to spread awareness of this hack to more places. We can't really fix this until we have fat pointers. I would rather have the declaration be accurate. If graphics users want to assume readnone as a performance hack until that is fixed, they can annotate every call site with readnone.

• region pointers cannot appear as flat pointers
• expand the tests, handle the cmpxchg case

Address review comment

LGTM

Good idea, but needs a fix I think.

I should clarify that I'm not opposed to this kind of change in the long run, but as-is it needs a careful look at the performance implications.

What does this actually fix?

I don't quite understand why you think this can't be writeonly? (The flip side is, I don't see how having this writeonly would help codegen)

Wrap LDS TargetGlobalAddress nodes in an AMDGPUISD::LDS node that is
selected to S_MOV/V_MOV. This should address the concerns about having
more machine nodes during SelectionDAG, and as a nice side bonus it
actually improves the quality of known-bits information by adding
alignment information as well.

Rebase, address some of the comments.

Significant update to this patch to make .amdgpu_lds work more
like common symbols:

Thanks, this is very helpful!

LGTM