AMDGPU: Treat texture gather instructions more like other MIMG instructions

[nhaehnle]

Summary

Setting MIMG to 0 has a bunch of unexpected side effects, including that
isVMEM returns false which leads to incorrect treatment in the hazard
recognizer. The reason I noticed it is that it also leads to incorrect
treatment in VGPR-to-SGPR copies, which is one cause of the referenced bug.

The only reason why MIMG was set to 0 is to signal the special handling of
dmasks, but that can be checked differently.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96877

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

3136 ↗

(On Diff #63483)

I don't understand why the check for hasPostISelHook is here. It shouldn't be a relevant property to check here

test/CodeGen/AMDGPU/llvm.SI.gather4.ll

468–470 ↗

(On Diff #63483)

Needs some check lines

[nhaehnle]

lib/Target/AMDGPU/SIISelLowering.cpp

3136 ↗

(On Diff #63483)

What we really want to check is whether the MIMG instructions' dmask has the default treatment (i.e. bitfield of returned components) as opposed to the special gather4 treatment.

For MIMG instructions, this coincides precisely with the hasPostISelHook, because fixing up the dmask is precisely what the PostISelHook is used for.

I could add an entry to the TSFlags for this bit, though I didn't do it because the information is redundant. Would you prefer the TSFlags solution?

[nhaehnle]

Add some CHECK lines to the new test.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

3136–3137 ↗

(On Diff #63551)

Is dmask an operand? can you just see if getNamedOperandIdx says it has the operand?

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

3136–3137 ↗

(On Diff #63551)

But yes, a new flag would be better than repurposing this one

[nhaehnle]

The trouble is that the operand is named "dmask" for all image sample _and_
gather4 instructions :)

Added the Gather4 flag.

[arsenm]

lib/Target/AMDGPU/SIISelLowering.cpp

3137 ↗

(On Diff #63575)

This should be a new check in SIInstrInfo. Would mayLoad be better than !mayStore?

[nhaehnle]

Add isGather4 in SIInstrInfo.

!mayStore is the correct check, because we need to account for image
atomics here.

[arsenm]

LGTM

llvm/trunk/lib/Target/AMDGPU/SIDefines.h

enum {
   MUBUF = 1 << 16,
   MTBUF = 1 << 17,
   SMRD = 1 << 18,
   DS = 1 << 19,
   MIMG = 1 << 20,
   FLAT = 1 << 21,
   WQM = 1 << 22,
   VGPRSpill = 1 << 23,
-  VOPAsmPrefer32Bit = 1 << 24
+  VOPAsmPrefer32Bit = 1 << 24,
+  Gather4 = 1 << 25
 };
 }
 
 namespace llvm {
 namespace AMDGPU {
   enum OperandType {
     /// Operand with register or 32-bit immediate
     OPERAND_REG_IMM32 = MCOI::OPERAND_FIRST_TARGET,

llvm/trunk/lib/Target/AMDGPU/SIISelLowering.cpp

 }
 
 /// \brief Fold the instructions after selecting them.
 SDNode *SITargetLowering::PostISelFolding(MachineSDNode *Node,
                                           SelectionDAG &DAG) const {
   const SIInstrInfo *TII = getSubtarget()->getInstrInfo();
   unsigned Opcode = Node->getMachineOpcode();
 
-  if (TII->isMIMG(Opcode) && !TII->get(Opcode).mayStore())
+  if (TII->isMIMG(Opcode) && !TII->get(Opcode).mayStore() &&
+      !TII->isGather4(Opcode))
     adjustWritemask(Node, DAG);
 
   if (Opcode == AMDGPU::INSERT_SUBREG ||
       Opcode == AMDGPU::REG_SEQUENCE) {
     legalizeTargetIndependentNode(Node, DAG);
     return Node;
   }
   return Node;

llvm/trunk/lib/Target/AMDGPU/SIInstrFormats.td

class InstSI <dag outs, dag ins, string asm = "",
   field bits<1> FLAT = 0;
   field bits<1> WQM = 0;
   field bits<1> VGPRSpill = 0;
 
   // This bit tells the assembler to use the 32-bit encoding in case it
   // is unable to infer the encoding from the operands.
   field bits<1> VOPAsmPrefer32Bit = 0;
 
+  field bits<1> Gather4 = 0;
+
   // These need to be kept in sync with the enum in SIInstrFlags.
   let TSFlags{0} = VM_CNT;
   let TSFlags{1} = EXP_CNT;
   let TSFlags{2} = LGKM_CNT;
 
   let TSFlags{3} = SALU;
   let TSFlags{4} = VALU;
 
class InstSI <dag outs, dag ins, string asm = "",
   let TSFlags{17} = MTBUF;
   let TSFlags{18} = SMRD;
   let TSFlags{19} = DS;
   let TSFlags{20} = MIMG;
   let TSFlags{21} = FLAT;
   let TSFlags{22} = WQM;
   let TSFlags{23} = VGPRSpill;
   let TSFlags{24} = VOPAsmPrefer32Bit;
+  let TSFlags{25} = Gather4;
 
   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);

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.h

public:
   static bool isMIMG(const MachineInstr &MI) {
     return MI.getDesc().TSFlags & SIInstrFlags::MIMG;
   }
 
   bool isMIMG(uint16_t Opcode) const {
     return get(Opcode).TSFlags & SIInstrFlags::MIMG;
   }
 
+  static bool isGather4(const MachineInstr &MI) {
+    return MI.getDesc().TSFlags & SIInstrFlags::Gather4;
+  }
+
+  bool isGather4(uint16_t Opcode) const {
+    return get(Opcode).TSFlags & SIInstrFlags::Gather4;
+  }
+
   static bool isFLAT(const MachineInstr &MI) {
     return MI.getDesc().TSFlags & SIInstrFlags::FLAT;
   }
 
   bool isFLAT(uint16_t Opcode) const {
     return get(Opcode).TSFlags & SIInstrFlags::FLAT;
   }
 

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.td

class MIMG_Gather_Helper <bits<7> op, string asm,
   let mayStore = 0;
 
   // DMASK was repurposed for GATHER4. 4 components are always
   // returned and DMASK works like a swizzle - it selects
   // the component to fetch. The only useful DMASK values are
   // 1=red, 2=green, 4=blue, 8=alpha. (e.g. 1 returns
   // (red,red,red,red) etc.) The ISA document doesn't mention
   // this.
-  // Therefore, disable all code which updates DMASK by setting these two:
-  let MIMG = 0;
+  // Therefore, disable all code which updates DMASK by setting this:
+  let Gather4 = 1;
   let hasPostISelHook = 0;
   let WQM = wqm;
 
   let isAsmParserOnly = 1; // TBD: fix it later
 }
 
 multiclass MIMG_Gather_Src_Helper <bits<7> op, string asm,
                                     RegisterClass dst_rc,

llvm/trunk/test/CodeGen/AMDGPU/llvm.SI.gather4.ll

main_body:
   %r0 = extractelement <4 x float> %r, i32 0
   %r1 = extractelement <4 x float> %r, i32 1
   %r2 = extractelement <4 x float> %r, i32 2
   %r3 = extractelement <4 x float> %r, i32 3
   call void @llvm.SI.export(i32 15, i32 1, i32 1, i32 0, i32 1, float %r0, float %r1, float %r2, float %r3)
   ret void
 }
 
-
+;CHECK-LABEL: {{^}}gather4_sgpr_bug:
+;
+; This crashed at some point due to a bug in FixSGPRCopies. Derived from the
+; report in https://bugs.freedesktop.org/show_bug.cgi?id=96877
+;
+;TODO: the readfirstlanes are unnecessary, see http://reviews.llvm.org/D22217
+;
+;CHECK: v_readfirstlane_b32 s[[LO:[0-9]+]], v{{[0-9]+}}
+;CHECK: v_readfirstlane_b32
+;CHECK: v_readfirstlane_b32
+;CHECK: v_readfirstlane_b32 s[[HI:[0-9]+]], v{{[0-9]+}}
+;CHECK: image_gather4_lz {{v\[[0-9]+:[0-9]+\]}}, {{v\[[0-9]+:[0-9]+\]}}, {{s\[[0-9]+:[0-9]+\]}}, s{{\[}}[[LO]]:[[HI]]] dmask:0x8
+define amdgpu_ps float @gather4_sgpr_bug() {
+main_body:
+  %tmp = load <4 x i32>, <4 x i32> addrspace(2)* undef, align 16
+  %tmp1 = insertelement <4 x i32> %tmp, i32 0, i32 0
+  %tmp2 = call <4 x float> @llvm.SI.gather4.lz.v2i32(<2 x i32> undef, <8 x i32> undef, <4 x i32> %tmp1, i32 8, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0, i32 0)
+  %tmp4 = extractelement <4 x float> %tmp2, i32 1
+  %tmp9 = fadd float undef, %tmp4
+  ret float %tmp9
+}
 
 declare <4 x float> @llvm.SI.gather4.v2i32(<2 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #0
 declare <4 x float> @llvm.SI.gather4.v4i32(<4 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #0
 declare <4 x float> @llvm.SI.gather4.cl.v4i32(<4 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #0
 declare <4 x float> @llvm.SI.gather4.l.v4i32(<4 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #0
 declare <4 x float> @llvm.SI.gather4.b.v4i32(<4 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #0
 declare <4 x float> @llvm.SI.gather4.b.cl.v4i32(<4 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #0
 declare <4 x float> @llvm.SI.gather4.b.cl.v8i32(<8 x i32>, <8 x i32>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32) #0