[AMDGPU] kernel regression after enabling `amdgpu-uniform-intrinsic-combine`

[raiseirql]

We (Modular) integrated LLVM commit 2108c623e618265c4146c405f196953a9c157e73 to our build which includes the changes to turn on amdgpu-uniform-intrinsic-combine by default (#162819).

The kernel located here regressed as a result: #166657 (comment)

Some of the readfirstlane intrinsics were removed from this kernel, but then later passes of the compiler ended up using VGPRs for some of the calculations, and this in turned triggered si-fix-sgpr-copies to generate code like this at every buffer load instruction:

.LBB0_9:                                ;   Parent Loop BB0_7 Depth=1
                                        ; =>  This Inner Loop Header: Depth=2
        v_readfirstlane_b32 s4, v0
        v_readfirstlane_b32 s5, v1
        v_readfirstlane_b32 s6, v254
        v_readfirstlane_b32 s7, v255
        v_cmp_eq_u64_e32 vcc, s[4:5], v[0:1]
        s_nop 0
        v_cmp_eq_u64_e64 s[2:3], s[6:7], v[254:255]
        s_and_b64 s[2:3], vcc, s[2:3]
        s_and_saveexec_b64 s[2:3], s[2:3]
        buffer_load_dwordx4 v[6:9], v10, s[4:7], s48 offen
        s_xor_b64 exec, exec, s[2:3]
        s_cbranch_execnz .LBB0_9
; %bb.10:                               ;   in Loop: Header=BB0_7 Depth=1
        s_cmp_lg_u32 s55, 0
        s_mov_b64 exec, s[38:39]
        s_cselect_b32 s55, 1, 0
        s_mov_b64 s[38:39], exec
.LBB0_11:              

We have worked around locally by turning off the pass, but the pass has value and we would like it to be enabled. Either the original determination that this was uniform was wrong or the instruction selection later should have picked a scalar form of some instruction to avoid generating the above sequence.

I was comparing the code from:

llc -O3 -mcpu=gfx950 mha.ll --amdgpu-enable-uniform-intrinsic-combine=false
llc -O3 -mcpu=gfx950 mha.ll --amdgpu-enable-uniform-intrinsic-combine=true

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

Author: Tracy Sharpe (raiseirql)

We (Modular) integrated LLVM commit `2108c623e618265c4146c405f196953a9c157e73` to our build which includes the changes to turn on `amdgpu-uniform-intrinsic-combine` by default (#162819).

The kernel located here regressed as a result: #166657 (comment)

Some of the readfirstlane intrinsics were removed from this kernel, but then later passes of the compiler ended up using VGPRs for some of the calculations, and this in turned triggered si-fix-sgpr-copies to generate code like this at every buffer load instruction:

.LBB0_9:                                ;   Parent Loop BB0_7 Depth=1
                                        ; =>  This Inner Loop Header: Depth=2
        v_readfirstlane_b32 s4, v0
        v_readfirstlane_b32 s5, v1
        v_readfirstlane_b32 s6, v254
        v_readfirstlane_b32 s7, v255
        v_cmp_eq_u64_e32 vcc, s[4:5], v[0:1]
        s_nop 0
        v_cmp_eq_u64_e64 s[2:3], s[6:7], v[254:255]
        s_and_b64 s[2:3], vcc, s[2:3]
        s_and_saveexec_b64 s[2:3], s[2:3]
        buffer_load_dwordx4 v[6:9], v10, s[4:7], s48 offen
        s_xor_b64 exec, exec, s[2:3]
        s_cbranch_execnz .LBB0_9
; %bb.10:                               ;   in Loop: Header=BB0_7 Depth=1
        s_cmp_lg_u32 s55, 0
        s_mov_b64 exec, s[38:39]
        s_cselect_b32 s55, 1, 0
        s_mov_b64 s[38:39], exec
.LBB0_11:              

We have worked around locally by turning off the pass, but the pass has value and we would like it to be enabled. Either the original determination that this was uniform was wrong or the instruction selection later should have picked a scalar form of some instruction to avoid generating the above sequence.

I was comparing the code from:

llc -O3 -mcpu=gfx950 mha.ll --amdgpu-enable-uniform-intrinsic-combine=false
llc -O3 -mcpu=gfx950 mha.ll --amdgpu-enable-uniform-intrinsic-combine=true

[jayfoad]

Just to confirm, the code is still functionally correct right? It's "only" a performance regression?

There is a known problem here, that si-fix-sgpr-copies will generate waterfall loops when a descriptor ends up in VGPRs, even if the compiler could prove that those VGPR values are uniform. But I'm not sure why #162819 would have triggered that problem in this case.

[raiseirql]

Combined with #166657, this resulted in a kernel crash/correctness issue, but absent that, this would have been just a performance regression.

One thing I had noticed before: the buffer loads at %2481, %2491, %2498, and %2505 are generating fine without the waterfall loop. However, the other buffer loads in the same basic block before this do have the waterfall loop. Before %2481, there is a:

  fence syncscope("workgroup") release ; comment this out
  tail call void @llvm.amdgcn.s.barrier()
  fence syncscope("workgroup") acquire ; comment this out

If I remove the fence instructions here, then the previous set of four buffer loads are then generated correctly. The barrier by itself seems fine (removing just . Continuing this up the block, then I can "fix" the previous instructions to not have the waterfall loop.

I don't know if the fences are required here for correctness, but does that give some clues as to what might be going wrong? It would make more sense if this was at the start of the block, so seems some analysis is broken. Looking at the buffer resource sgpr_128, it's not otherwise clear why this isn't scalar.

[jayfoad]

Combined with #166657, this resulted in a kernel crash/correctness issue, but absent that, this would have been just a performance regression.

Makes sense, thanks.

[jayfoad]

TL;DR: AMDGPUUniformIntrinsicCombine should not remove readfirstlane intrinsics, even if it can prove that they are semantically unnecessary because the input is uniform.

The problem is that waterfall loops are currently inserted by SIFixSGPRCopies (just after instruction selection) and it is done based on register banks not on uniformity analysis. There are plenty of reasons why a uniform value can end up in a VGPR, in which case SIFixSGPRCopies will insert an unnecessary (but supposedly harmless) waterfall loop. readfirstlane intrinsics provide a convenient way to force a known-uniform value into an SGPR, to avoid the unnecessary waterfall loop, and frontends sometimes insert them for exactly that reason. Having AMDGPUUniformIntrinsicCombine remove them defeats this purpose.

The proper way to fix this in the longer term is for waterfall loops to be generated during instruction selection and based on uniformity analysis, not on register banks. For SelectionDAG this probably involves selecting some kind of waterfall loop pseudo-instructions and then expanding them into real control flow loops in EmitInstrWithCustomInserter.

[jayfoad]

@PankajDwivedi-25

[PankajDwivedi-25]

If the issue is the only with readfirstlane, possibly I will stop optimizing it.

[PankajDwivedi-25]

PR:#166955 could be a temporary fix for the issue if its not same with other intrinsics.