This can be merged. I have a patch for Mesa.

@arsenm @foad Can we backport it to LLVM 14? LLVM 14 is unusable because of this bug.

I think we don't need the pass, but it's hard to tell.

I suppose the only difference this makes here is potentially in the counter value we wait for? I.e., we may wait for vmcnt(N) with N != 0?

The IR (NIR) that we translate to LLVM typically contains this sequence of instructions:

1. memory_barrier (fence)
2. control_barrier (s_barrier)

We are going to insert amdgcn.s.waitcnt instead of a fence because fences wait for memory stores, which we don't want.

In D120544#3486402, @nhaehnle wrote:

In D120544#3466638, @kerbowa wrote:

I believe HIP adds a release fence before the barrier and an acquire directly after. Would something like that make sense?

Yes, generally I expect that is what's required.

In D120544#3464886, @nhaehnle wrote:

Oh, I misunderstood what this patch does. It's possibly an unfortunate interaction between Mesa and LLVM, where Mesa assumed that the waitcnt was there, and your removal of it causes problems. In that case, can you please coordinate with Mesa to make sure it starts adding the required fence instructions in the LLVM IR? Cc @mareko

In D94645#2500467, @ruiling wrote:

BTW I felt the input in the name of llvm.amdgcn.init.exec.from.input some kind of means the argument is the function input argument in LLVM IR. I think we can clarify that the value to this intrinsic should be an function input argument. That is the only requirement I can see currently from frontend. So that we don't need to consider various hard to solve situations. @mareko What do you think?

This looks good to me, but I'm not very familiar with LLVM.

Using the new return behavior only for the calling conventions that Mesa doesn't use sounds like the right solution.

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

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.

In D84403#2254048, @foad wrote:

In D84403#2253762, @mareko wrote:

In D84403#2251541, @mbrkusanin wrote:

In D84403#2249309, @mareko wrote:

In D84403#2248880, @mbrkusanin wrote:

In D84403#2248536, @mareko wrote:

This breaks LDS. LLVMSetAlignment(inst, 4) on loads and stores has no effect. The IR says "align 4", yet the backend still selects b128.

On what subtargets? GFX9 and 10 should select b128 for align 4. That is the purpose of the patch. Are you saying it selects it for SI, CI or VI?

On GFX10. Apparently b128 with align 4 doesn't work there.

I've checked a couple Vulkan CTS tests that now produce b128 instructions for SDag and they work fine. I also did not find any regressions on others. Can you give us any more details? Or a test to reproduce the issue?

More information:

• (gfx9 hasn't been tested)
• gfx10.1 has the corruption in WGP mode only (CU mode works)
• gfx10.3 works

It looks like it's a gfx10.1 hw bug in WGP mode, so a fix or workaround is needed. The driver always uses WGP mode.

Do you know what the driver sets SH_MEM_CONFIG.alignment_mode to?

In D84403#2251541, @mbrkusanin wrote:

In D84403#2249309, @mareko wrote:

In D84403#2248880, @mbrkusanin wrote:

In D84403#2248536, @mareko wrote:

This breaks LDS. LLVMSetAlignment(inst, 4) on loads and stores has no effect. The IR says "align 4", yet the backend still selects b128.

On what subtargets? GFX9 and 10 should select b128 for align 4. That is the purpose of the patch. Are you saying it selects it for SI, CI or VI?

On GFX10. Apparently b128 with align 4 doesn't work there.

I've checked a couple Vulkan CTS tests that now produce b128 instructions for SDag and they work fine. I also did not find any regressions on others. Can you give us any more details? Or a test to reproduce the issue?

In D84403#2251541, @mbrkusanin wrote:

In D84403#2249309, @mareko wrote:

In D84403#2248880, @mbrkusanin wrote:

In D84403#2248536, @mareko wrote:

This breaks LDS. LLVMSetAlignment(inst, 4) on loads and stores has no effect. The IR says "align 4", yet the backend still selects b128.

On what subtargets? GFX9 and 10 should select b128 for align 4. That is the purpose of the patch. Are you saying it selects it for SI, CI or VI?

On GFX10. Apparently b128 with align 4 doesn't work there.

I've checked a couple Vulkan CTS tests that now produce b128 instructions for SDag and they work fine. I also did not find any regressions on others. Can you give us any more details? Or a test to reproduce the issue?

In D84403#2248880, @mbrkusanin wrote:

In D84403#2248536, @mareko wrote:

This breaks LDS. LLVMSetAlignment(inst, 4) on loads and stores has no effect. The IR says "align 4", yet the backend still selects b128.

On what subtargets? GFX9 and 10 should select b128 for align 4. That is the purpose of the patch. Are you saying it selects it for SI, CI or VI?

This breaks LDS. LLVMSetAlignment(inst, 4) on loads and stores has no effect. The IR says "align 4", yet the backend still selects b128.

Can you please cherry-pick this to the LLVM 11 branch?

Can you push this after (not before) LLVM 10 has been branched?

@nhaehnle

Would somebody please push this?

This breaks gfx8. Can you revert?

Yes.

It's supported by Mesa on gfx6-8. I'm OK with this though.

This has a possibly negative impact on Mesa. Both SGPR usage and SGPR spilling increased. I don't see anything suspicious in the generated assembly other than instructions being reordered. I'd like to better understand the side effects of this patch.

This breaks Mesa:
LLVM ERROR: Cannot select: intrinsic %llvm.amdgcn.s.buffer.load

FYI, if there is no feedback on January 15, I'll commit this to get it into LLVM 8.0.

Ping

Use report_fatal_error, add SourceOfDivergence lines

Any comments?

Add InstCombine tests.

If I make m0 integer, DS_ORDERED_COUNT won't be a mem node.

I might change the intrinsic to add the option to insert "s_and_saveexec s[N:M], 1" and "s_mov_b64 exec, s[N:M]" around the intrinsic to get an optimal single-lane block.

In D52944#1260081, @nhaehnle wrote:

In D52944#1259919, @arsenm wrote:

Does this need to be marked as isSourceOfDivergence?

Now that you mention it, yes, even though it's for stupid reasons: I believe the ds_ordered_count instruction executes only in a single lane, so it's intuitively a uniform operation; however, it returns its result only in lane 0, so it's formally non-uniform.

In D52907#1258465, @arsenm wrote:

In D52907#1258433, @mareko wrote:

In D52907#1258426, @arsenm wrote:

Why exactly? Is it possible the Mesa lds allocation isn’t aligning properly?

Since the issue only occurs on SI, I don't think Mesa is doing anything bad. Unless there is some LDS hw difference on SI...

I do remember one bug we have that may be related. We try to use the ds_read2_b32 with 4-byte signed trick on SI, without checking that we can use the offsets if the base address isn't known positive

I'm concerned that x8 and x16 loads will significantly increase SGPR usage and therefore SGPR spilling. We have a shader database with over 70 games and benchmarks and I guess the results will not be good after this is committed.

In D52907#1258426, @arsenm wrote:

Why exactly? Is it possible the Mesa lds allocation isn’t aligning properly?

Multi-dword LDS opcodes seem to be the culprit.

AMDGPU: Add a fast path for icmp.i1(src, false, NE)

In D52907#1256202, @arsenm wrote:

This isn't a correct fix. If there's an issue with 64-bit DS instructions, it's a lowering problem. If we can't use them for some reason, changing this here might be a helpful heuristic but as-is this is not a real fix

On a related note, another way to decrease VGPR usage is to fold immediates with more than 1 uses. The backend currently folds immediates with only 1 use.

What happens if a shader already does "if (threadID == 0) { do_atomic(); }"? Is the optimization skipped in this case?

In D52060#1234370, @arsenm wrote:

Should the instcombine part change also to allow creation of i1 uses?

This fixes GPU hangs with OpenGL bindless handle arithmetic.

@arsenm Where do you have the patches that preserve NUW?

We can ignore old Mesa + new LLVM, because LLVM 7 is the first release to have 32-bit pointers, and I think we can fix that before release.

In D51098#1209699, @tpr wrote:

Marek, I will correct the spelling of your name in the commit message when I land this. :-)

Accepted.

The patch is missing a test. shader-db can reproduce it.

In D49065#1161236, @michel.daenzer wrote:

In D49065#1161120, @mareko wrote:

This patch along with the prerequisite patch doesn't break Mesa.

What's the prerequisite patch?

This patch along with the prerequisite patch doesn't break Mesa.

This is a no-op change, right? Because the previous code also works.

FYI, this bug is also reproducible on Mesa OpenGL now.

LGTM.

In D46992#1105636, @rampitec wrote:

In D46992#1105635, @mareko wrote:

How does this pass affect shaders that use a lot of memory instructions but no pointers?

Can you give an example? What is a memory instruction without a pointer? As you may see, pass processes something which can cast to load, store, atomic or memory intrinsic. Everything else considered an ordinary instruction. For example if have an image in mind it is conservatively not considered memory instruction.

How does this pass affect shaders that use a lot of memory instructions but no pointers?

How can UMDs disable this optimization?

Committed.

In D44401#1040175, @arsenm wrote:

I think the unit change is a hint we should use different intrinsics for this