[AMDGPU] Fixed non-uniform addr64 MUBUF in shader

[tpr]

Summary

SIInstrInfo::legalizeOperands was assuming that it never sees an addr64
MUBUF in a shader. But it is generated from a global load, which LLPC
uses for a load from a read-only global variable.

Fixed to detect the addr64 case, and then legalize in the same way as in
compute.

Change-Id: I8aee2a0dc37d2061de445c30c7510e40b3e52f3c

[tpr]

lib/Target/AMDGPU/SIInstrInfo.cpp

3666

OK, so that check doesn't actually work. I'll push a revised fix in a bit.

[tpr]

V2: Fixed check for whether MUBUF/MTBUF is addr64

[tpr]

V3: Fixed the comment as well.

[tpr]

Pushed revised fix.

[arsenm]

lib/Target/AMDGPU/SIInstrInfo.cpp

3667

I don't like that this was checking the calling convention at all. Can we just eliminate that check altogether?

3669

You should be able to avoid adding isAddr64 by just moving the getNamedOperand call for vaddr below up here

[tpr]

lib/Target/AMDGPU/SIInstrInfo.cpp

3667

So (from a compute point of view) you think it is ok for all non-addr64 cases of MUBUF/MTBUF to go in here? I think that is probably ok, but then I'm not sure why Nicolai added the calling convention check in the first place.

3669

I tried that, and I got false positives on offen/idxen/bothen variants.

[arsenm]

lib/Target/AMDGPU/SIInstrInfo.cpp

3667

The main concerning case I'm aware of with anything touching these is for anything using the add-tid bit (and maybe strided access). In any case, the calling convention isn't actually the property that matters

3669

Oh right, the operand name is still the same for either

[tpr]

lib/Target/AMDGPU/SIInstrInfo.cpp

3667

So you want to replace the calling convention check with something else, but I don't think it is entirely clear yet what the check should be. Can we leave that to a subsequent change? My change here did not add it in the first place.

[tpr]

I'm abandoning this. I have a much better fix for my problem, now I have discovered that I can get at divergence information in instruction selection:
https://reviews.llvm.org/D47383

lib/Target/AMDGPU/BUFInstructions.td

: MTBUF_Pseudo<opName,
                  (outs vdataClass:$vdata),
                  getMTBUFIns<addrKindCopy>.ret,
                  " $vdata, " # getMTBUFAsmOps<addrKindCopy>.ret # "$glc$slc$tfe",
                  pattern>,
     MTBUF_SetupAddr<addrKindCopy> {
   let PseudoInstr = opName # "_" # getAddrName<addrKindCopy>.ret;
   let mayLoad = 1;
   let mayStore = 0;
+  let IsAddr64 = !if(addr64, 1, 0);
 }
 
 multiclass MTBUF_Pseudo_Loads<string opName, RegisterClass vdataClass,
                               ValueType load_vt = i32,
                               SDPatternOperator ld = null_frag> {
 
   def _OFFSET : MTBUF_Load_Pseudo <opName, BUFAddrKind.Offset, vdataClass,
     [(set load_vt:$vdata,
: MTBUF_Pseudo<opName,
                  (outs),
                  getMTBUFIns<addrKindCopy, [vdataClassCopy]>.ret,
                  " $vdata, " # getMTBUFAsmOps<addrKindCopy>.ret # "$glc$slc$tfe",
                  pattern>,
     MTBUF_SetupAddr<addrKindCopy> {
   let PseudoInstr = opName # "_" # getAddrName<addrKindCopy>.ret;
   let mayLoad = 0;
   let mayStore = 1;
+  let IsAddr64 = !if(addr64, 1, 0);
 }
 
 multiclass MTBUF_Pseudo_Stores<string opName, RegisterClass vdataClass,
                                ValueType store_vt = i32,
                                SDPatternOperator st = null_frag> {
 
   def _OFFSET : MTBUF_Store_Pseudo <opName, BUFAddrKind.Offset, vdataClass,
     [(st store_vt:$vdata, (MUBUFOffset v4i32:$srsrc, i32:$soffset,
class MUBUF_Load_Pseudo <string opName,
 
   let Constraints = !if(HasTiedDest, "$vdata = $vdata_in", "");
   let mayLoad = 1;
   let mayStore = 0;
   let maybeAtomic = 1;
   let Uses = !if(isLds, [EXEC, M0], [EXEC]);
   let has_tfe = !if(isLds, 0, 1);
   let lds = isLds;
+  let IsAddr64 = !if(addr64, 1, 0);
 }
 
 // FIXME: tfe can't be an operand because it requires a separate
 // opcode because it needs an N+1 register class dest register.
 multiclass MUBUF_Pseudo_Loads<string opName, RegisterClass vdataClass,
                               ValueType load_vt = i32,
                               SDPatternOperator ld = null_frag,
                               bit TiedDest = 0,
: MUBUF_Pseudo<opName,
                  getMUBUFIns<addrKindCopy, [vdataClassCopy]>.ret,
                  " $vdata, " # getMUBUFAsmOps<addrKindCopy>.ret # "$glc$slc$tfe",
                  pattern>,
     MUBUF_SetupAddr<addrKindCopy> {
   let PseudoInstr = opName # "_" # getAddrName<addrKindCopy>.ret;
   let mayLoad = 0;
   let mayStore = 1;
   let maybeAtomic = 1;
+  let IsAddr64 = !if(addr64, 1, 0);
 }
 
 multiclass MUBUF_Pseudo_Stores<string opName, RegisterClass vdataClass,
                                ValueType store_vt = i32,
                                SDPatternOperator st = null_frag> {
 
   def _OFFSET : MUBUF_Store_Pseudo <opName, BUFAddrKind.Offset, vdataClass,
     [(st store_vt:$vdata, (MUBUFOffset v4i32:$srsrc, i32:$soffset,

lib/Target/AMDGPU/SIDefines.h

// TODO: Should this be spilt into VOP3 a and b?
   // Clamps hi component of register.
   // ClampLo and ClampHi set for packed clamp.
   ClampHi = UINT64_C(1) << 48,
 
   // Is a packed VOP3P instruction.
   IsPacked = UINT64_C(1) << 49,
 
   // "d16" bit set or not.
-  D16 = UINT64_C(1) << 50
+  D16 = UINT64_C(1) << 50,
+
+  // Is MUBUF/MTBUF in addr64 mode.
+  IsAddr64 = UINT64_C(1) << 51
+
 };
 
 // v_cmp_class_* etc. use a 10-bit mask for what operation is checked.
 // The result is true if any of these tests are true.
 enum ClassFlags {
   S_NAN = 1 << 0,        // Signaling NaN
   Q_NAN = 1 << 1,        // Quiet NaN
   N_INFINITY = 1 << 2,   // Negative infinity

lib/Target/AMDGPU/SIInstrFormats.td

class InstSI <dag outs, dag ins, string asm = "",
   field bit ClampHi = 0;
 
   // This bit indicates that this is a packed VOP3P instruction
   field bit IsPacked = 0;
 
   // This bit indicates that this is a D16 instruction.
   field bit D16 = 0;
 
+  // This bit indicates that an MUBUF/MTBUF instruction is addr64.
+  field bit IsAddr64 = 0;
+
   // These need to be kept in sync with the enum in SIInstrFlags.
   let TSFlags{0} = SALU;
   let TSFlags{1} = VALU;
 
   let TSFlags{2} = SOP1;
   let TSFlags{3} = SOP2;
   let TSFlags{4} = SOPC;
   let TSFlags{5} = SOPK;
class InstSI <dag outs, dag ins, string asm = "",
   let TSFlags{45} = FPClamp;
   let TSFlags{46} = IntClamp;
   let TSFlags{47} = ClampLo;
   let TSFlags{48} = ClampHi;
 
   let TSFlags{49} = IsPacked;
 
   let TSFlags{50} = D16;
+  let TSFlags{51} = IsAddr64;
 
   let SchedRW = [Write32Bit];
 
   field bits<1> DisableSIDecoder = 0;
   field bits<1> DisableVIDecoder = 0;
   field bits<1> DisableDecoder = 0;
 
   let isAsmParserOnly = !if(!eq(DisableDecoder{0}, {0}), 0, 1);

lib/Target/AMDGPU/SIInstrInfo.h

public:
   bool isD16(uint16_t Opcode) const {
     return get(Opcode).TSFlags & SIInstrFlags::D16;
   }
 
   static bool isFLAT(const MachineInstr &MI) {
     return MI.getDesc().TSFlags & SIInstrFlags::FLAT;
   }
 
+  static bool isAddr64(const MachineInstr &MI) {
+    return MI.getDesc().TSFlags & SIInstrFlags::IsAddr64;
+  }
+
   // Is a FLAT encoded instruction which accesses a specific segment,
   // i.e. global_* or scratch_*.
   static bool isSegmentSpecificFLAT(const MachineInstr &MI) {
     auto Flags = MI.getDesc().TSFlags;
     return (Flags & SIInstrFlags::FLAT) && !(Flags & SIInstrFlags::LGKM_CNT);
   }
 
   // Any FLAT encoded instruction, including global_* and scratch_*.

lib/Target/AMDGPU/SIInstrInfo.cpp

if (MI.getOpcode() == AMDGPU::SI_INIT_M0) {
     MachineOperand &Src = MI.getOperand(0);
     if (Src.isReg() && RI.hasVGPRs(MRI.getRegClass(Src.getReg())))
       Src.setReg(readlaneVGPRToSGPR(Src.getReg(), MI, MRI));
     return;
   }
 
   // Legalize MIMG and MUBUF/MTBUF for shaders.
   //
-  // Shaders only generate MUBUF/MTBUF instructions via intrinsics or via
-  // scratch memory access. In both cases, the legalization never involves
-  // conversion to the addr64 form.
+  // Shaders only generate non-addr64 MUBUF/MTBUF instructions via intrinsics
+  // or via scratch memory access. In both cases, the legalization never
+  // involves conversion to the addr64 form.
   if (isMIMG(MI) ||
       (AMDGPU::isShader(MF.getFunction().getCallingConv()) &&
-       (isMUBUF(MI) || isMTBUF(MI)))) {
+       (isMUBUF(MI) || isMTBUF(MI))
+       && !isAddr64(MI))) {
     MachineOperand *SRsrc = getNamedOperand(MI, AMDGPU::OpName::srsrc);
     if (SRsrc && !RI.isSGPRClass(MRI.getRegClass(SRsrc->getReg()))) {
       unsigned SGPR = readlaneVGPRToSGPR(SRsrc->getReg(), MI, MRI);
       SRsrc->setReg(SGPR);
     }
 
     MachineOperand *SSamp = getNamedOperand(MI, AMDGPU::OpName::ssamp);
     if (SSamp && !RI.isSGPRClass(MRI.getRegClass(SSamp->getReg()))) {

test/CodeGen/AMDGPU/shader-addr64-nonuniform.ll

+; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx700 -verify-machineinstrs <%s | FileCheck -enable-var-scope -check-prefix=GCN -check-prefix=SICI %s
+
+; GCN-LABEL: {{^}}main:
+; GCN-NOT: readfirstlane
+; SICI: buffer_load_dwordx4 {{.*}} addr64
+
+@indexable = internal unnamed_addr addrspace(1) constant [6 x <3 x float>] [<3 x float> <float 1.000000e+00, float 0.000000e+00, float 0.000000e+00>, <3 x float> <float 0.000000e+00, float 1.000000e+00, float 0.000000e+00>, <3 x float> <float 0.000000e+00, float 0.000000e+00, float 1.000000e+00>, <3 x float> <float 0.000000e+00, float 1.000000e+00, float 1.000000e+00>, <3 x float> <float 1.000000e+00, float 0.000000e+00, float 1.000000e+00>, <3 x float> <float 1.000000e+00, float 1.000000e+00, float 0.000000e+00>]
+
+define amdgpu_ps float @main(i32 %arg18) {
+.entry:
+  %tmp31 = sext i32 %arg18 to i64
+  %tmp32 = getelementptr [6 x <3 x float>], [6 x <3 x float>] addrspace(1)* @indexable, i64 0, i64 %tmp31
+  %tmp33 = load <3 x float>, <3 x float> addrspace(1)* %tmp32, align 16
+  %tmp34 = extractelement <3 x float> %tmp33, i32 0
+  ret float %tmp34
+}
+