AMDGPU generates v_cndmask/readfirstlane for uniform select

[jayfoad]

Test case:

define amdgpu_ps i32 @_amdgpu_ps_main(i32 inreg %arg) {
bb:
  %i = icmp eq i32 %arg, 0
  %i1 = zext i1 %i to i64
  %i2 = getelementptr i8, ptr addrspace(4) null, i64 %i1
  %i3 = load i32, ptr addrspace(4) %i2, align 8
  ret i32 %i3
}

If I compile with llc -march=amdgcn -mcpu=gfx900 I get:

_amdgpu_ps_main:                        ; @_amdgpu_ps_main
; %bb.0:                                ; %bb
	s_cmp_eq_u32 s0, 0
	s_cselect_b64 s[2:3], -1, 0
	v_cndmask_b32_e64 v0, 0, 1, s[2:3]
	s_mov_b32 s1, 0
	v_readfirstlane_b32 s0, v0
	s_load_dword s0, s[0:1], 0x0
	s_waitcnt lgkmcnt(0)
	; return to shader part epilog

All computations are uniform, so the use of v_cndmask_b32_e64 and v_readfirstlane_b32 is wasteful and inefficient.

[llvmbot]

@llvm/issue-subscribers-backend-amdgpu

[jayfoad]

@alex-t FYI

[Pierre-vh]

The DAG pattern responsible for this select is:

class ZExt_i64_i1_Pat <SDNode ext> : GCNPat <
  (i64 (ext i1:$src)),
    (REG_SEQUENCE VReg_64,
      (V_CNDMASK_B32_e64 /*src0mod*/(i32 0), /*src0*/(i32 0),
                         /*src1mod*/(i32 0), /*src1*/(i32 1), $src),
      sub0, (S_MOV_B32 (i32 0)), sub1)
>;

GlobalISel gets it right because selection is done manually in selectG_SZA_EXT and uses S_BFE_U64 directly

[Pierre-vh]

I'm not sure if we can port the GISel logic to TableGen easily, but it should be fairly easy to port at least a subset of that logic (the part that can generate S_BFE) to AMDGPUISelDAGtoDAG. If adding C++ selection logic there is acceptable then I don't mind preparing a patch.

[jayfoad]

Why do we need C++? Can't we just add a pattern for the uniform case using something like UniformUnaryFrag<ext>?

[Pierre-vh]

I think doing it in TableGen is doable but I'm not sure how i1s work in DAG/GISel. In both cases, do they become SReg_64 with the value being in sub0?
That pattern seems to work for the DAG case, I need to check if breaks something GISel:

def : GCNPat <
  (i64 (UniformUnaryFrag<zext> i1:$src)),
  (S_BFE_U64 SReg_64:$src, (i32 0x10000))
>;

[Pierre-vh]

Ah I remember (I think), i1 becomes a 32/64 bit reg depending on the wave size.
I got the following pattern so far:

multiclass ZExt_i64_i1_Pat <SDNode ext> {
  def: GCNPat <
    (i64 (ext i1:$src)),
      (REG_SEQUENCE VReg_64,
        (V_CNDMASK_B32_e64 /*src0mod*/(i32 0), /*src0*/(i32 0),
                          /*src1mod*/(i32 0), /*src1*/(i32 1), $src),
        sub0, (S_MOV_B32 (i32 0)), sub1)
  >;

  let WaveSizePredicate = isWave32 in
  def : GCNPat <
    (i64 (UniformUnaryFrag<ext> i1:$src)),
    (S_BFE_U64 (REG_SEQUENCE SReg_64, SReg_32:$src, sub0, (i32 (IMPLICIT_DEF)), sub1), (i32 0x10000))
  >;

  let WaveSizePredicate = isWave64 in
  def : GCNPat <
    (i64 (UniformUnaryFrag<ext> i1:$src)),
    (S_BFE_U64 SReg_64:$src, (i32 0x10000))
  >;
}

Looks like it makes GlobalISel crash so that needs looking into. There's also some regressions because sometimes the result of the uniform zext is used by a divergent instruction, so there's an extra v_mov needed in those cases. Can we do something about that? Maybe a SIFoldOperand fold for S_BFE -> V_BFE (not sure if it's interchangeable)

[jayfoad]

There's also some regressions because sometimes the result of the uniform zext is used by a divergent instruction, so there's an extra v_mov needed in those cases. Can we do something about that?

That's the job of SIFixSGPRCopies, to make intelligent decisions about when to convert an SALU instruction to VALU to avoid copies.

Maybe a SIFoldOperand fold for S_BFE -> V_BFE (not sure if it's interchangeable)

moveToVALU should be able to do this but yeah it is a tricky case because the operands are not the same.

[nhaehnle]

Ah I remember (I think), i1 becomes a 32/64 bit reg depending on the wave size.

Shouldn't this depend on whether the i1 is uniform or not? A uniform i1 can just become a 32-bit SGPR with values 0 or 1. A divergent i1 needs to become a lanemask.

[jayfoad]

Shouldn't this depend on whether the i1 is uniform or not?

We're specifically only trying to handle the uniform case here.

[Pierre-vh]

Review: https://reviews.llvm.org/D144715