[AMDGPU] Avoid using divergent value in mubuf addr64 descriptor

[tpr]

Summary

This fixes a problem where a load from global+idx generated incorrect
code on <=gfx7 when the index is divergent.

Change-Id: Ib4d177d6254b1dd3f8ec0203fdddec94bd8bc5ed

[arsenm]

The test seems pretty lacking. There are a lot more combinations of divergent and non-divergent changed here that aren't tested

lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

992

SReg_64_XEXEC

[arsenm]

lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

992

Probably should also create a helper that the constant lowering can use as well since this is basically the same

[tpr]

V2: s_mov_b64 helper func; improved test

[tpr]

lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

992

I left the SGPR_64RegClassID as that's what the existing code for a 64 bit constant load has. I have done the suggested helper func.

[nhaehnle]

Thanks. I feel like some of this could perhaps be improved with a computeKnownBits if there is a 64-bit uniform base and a 32-bit non-uniform offset, but that doesn't have to be part of this change.

lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

379–383

Have you run clang-format on this? It looks a bit off.

986

Spurious whitespace change.

1051–1066

This has a lot of redundancy with the isBaseWithConstantOffset case above. Perhaps the cases can be combined?

[tpr]

V3: minor formatting; commoned up two legs of fix

[tpr]

V4: fixed bugs in commoning two legs

[tpr]

Ping.

[nhaehnle]

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

26–34

Could you please add a similar test-case, with a non-uniform i64 %arg18 and %offset a constant? I don't think this case is covered by tests, and I'm not sure that the code would do the right thing for that case, where I think Addr64 would also be needed.

[tpr]

V5: Added extra test suggested in review comment, and fixed the bug it showed up.

[tpr]

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

26–34

Good spot; that case was not covered.

[nhaehnle]

Great, thanks!

lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp

 
 private:
   std::pair<SDValue, SDValue> foldFrameIndex(SDValue N) const;
   bool isNoNanSrc(SDValue N) const;
   bool isInlineImmediate(const SDNode *N) const;
 
   bool isUniformBr(const SDNode *N) const;
 
+  MachineSDNode *buildSMovImm64(SDLoc &DL, uint64_t Val, EVT VT) const;
+
   SDNode *glueCopyToM0(SDNode *N) const;
 
   const TargetRegisterClass *getOperandRegClass(SDNode *N, unsigned OpNo) const;
   virtual bool SelectADDRVTX_READ(SDValue Addr, SDValue &Base, SDValue &Offset);
   virtual bool SelectADDRIndirect(SDValue Addr, SDValue &Base, SDValue &Offset);
   bool isDSOffsetLegal(const SDValue &Base, unsigned Offset,
                        unsigned OffsetBits) const;
   bool SelectDS1Addr1Offset(SDValue Ptr, SDValue &Base, SDValue &Offset) const;
SDNode *AMDGPUDAGToDAGISel::glueCopyToM0(SDNode *N) const {
   SmallVector <SDValue, 8> Ops;
   for (unsigned i = 0, e = N->getNumOperands(); i != e; ++i) {
      Ops.push_back(N->getOperand(i));
   }
   Ops.push_back(Glue);
   return CurDAG->MorphNodeTo(N, N->getOpcode(), N->getVTList(), Ops);
 }
 
+MachineSDNode *AMDGPUDAGToDAGISel::buildSMovImm64(SDLoc &DL, uint64_t Imm,
+                                                  EVT VT) const {
+  SDNode *Lo = CurDAG->getMachineNode(
+      AMDGPU::S_MOV_B32, DL, MVT::i32,
+      CurDAG->getConstant(Imm & 0xFFFFFFFF, DL, MVT::i32));
+  SDNode *Hi =
+      CurDAG->getMachineNode(AMDGPU::S_MOV_B32, DL, MVT::i32,
+                             CurDAG->getConstant(Imm >> 32, DL, MVT::i32));
+  const SDValue Ops[] = {
+      CurDAG->getTargetConstant(AMDGPU::SReg_64RegClassID, DL, MVT::i32),
+      SDValue(Lo, 0), CurDAG->getTargetConstant(AMDGPU::sub0, DL, MVT::i32),
+      SDValue(Hi, 0), CurDAG->getTargetConstant(AMDGPU::sub1, DL, MVT::i32)};
+
+  return CurDAG->getMachineNode(TargetOpcode::REG_SEQUENCE, DL, VT, Ops);
+}
+
 static unsigned selectSGPRVectorRegClassID(unsigned NumVectorElts) {
   switch (NumVectorElts) {
   case 1:
     return AMDGPU::SReg_32_XM0RegClassID;
   case 2:
     return AMDGPU::SReg_64RegClassID;
   case 4:
     return AMDGPU::SReg_128RegClassID;
case ISD::ConstantFP: {
     if (ConstantFPSDNode *FP = dyn_cast<ConstantFPSDNode>(N))
       Imm = FP->getValueAPF().bitcastToAPInt().getZExtValue();
     else {
       ConstantSDNode *C = cast<ConstantSDNode>(N);
       Imm = C->getZExtValue();
     }
 
     SDLoc DL(N);
-    SDNode *Lo = CurDAG->getMachineNode(AMDGPU::S_MOV_B32, DL, MVT::i32,
-                                CurDAG->getConstant(Imm & 0xFFFFFFFF, DL,
-                                                    MVT::i32));
-    SDNode *Hi = CurDAG->getMachineNode(AMDGPU::S_MOV_B32, DL, MVT::i32,
-                                CurDAG->getConstant(Imm >> 32, DL, MVT::i32));
-    const SDValue Ops[] = {
-      CurDAG->getTargetConstant(AMDGPU::SReg_64RegClassID, DL, MVT::i32),
-      SDValue(Lo, 0), CurDAG->getTargetConstant(AMDGPU::sub0, DL, MVT::i32),
-      SDValue(Hi, 0), CurDAG->getTargetConstant(AMDGPU::sub1, DL, MVT::i32)
-    };
-
-    ReplaceNode(N, CurDAG->getMachineNode(TargetOpcode::REG_SEQUENCE, DL,
-                                          N->getValueType(0), Ops));
+    ReplaceNode(N, buildSMovImm64(DL, Imm, N->getValueType(0)));
     return;
   }
   case ISD::LOAD:
   case ISD::STORE:
   case ISD::ATOMIC_LOAD:
   case ISD::ATOMIC_STORE: {
     N = glueCopyToM0(N);
     break;
bool AMDGPUDAGToDAGISel::SelectDS64Bit4ByteAligned(SDValue Addr, SDValue &Base,
   // FIXME: This is broken on SI where we still need to check if the base
   // pointer is positive here.
   Base = Addr;
   Offset0 = CurDAG->getTargetConstant(0, DL, MVT::i8);
   Offset1 = CurDAG->getTargetConstant(1, DL, MVT::i8);
   return true;
 }
 
 bool AMDGPUDAGToDAGISel::SelectMUBUF(SDValue Addr, SDValue &Ptr,
                                      SDValue &VAddr, SDValue &SOffset,
                                      SDValue &Offset, SDValue &Offen,
                                      SDValue &Idxen, SDValue &Addr64,
                                      SDValue &GLC, SDValue &SLC,
                                      SDValue &TFE) const {
   // Subtarget prefers to use flat instruction
   if (Subtarget->useFlatForGlobal())
     return false;
 
   SDLoc DL(Addr);
 
   if (!GLC.getNode())
     GLC = CurDAG->getTargetConstant(0, DL, MVT::i1);
   if (!SLC.getNode())
     SLC = CurDAG->getTargetConstant(0, DL, MVT::i1);
   TFE = CurDAG->getTargetConstant(0, DL, MVT::i1);
 
   Idxen = CurDAG->getTargetConstant(0, DL, MVT::i1);
   Offen = CurDAG->getTargetConstant(0, DL, MVT::i1);
   Addr64 = CurDAG->getTargetConstant(0, DL, MVT::i1);
   SOffset = CurDAG->getTargetConstant(0, DL, MVT::i32);
 
+  ConstantSDNode *C1 = nullptr;
+  SDValue N0 = Addr;
   if (CurDAG->isBaseWithConstantOffset(Addr)) {
-    SDValue N0 = Addr.getOperand(0);
-    SDValue N1 = Addr.getOperand(1);
-    ConstantSDNode *C1 = cast<ConstantSDNode>(N1);
+    C1 = cast<ConstantSDNode>(Addr.getOperand(1));
+    if (isUInt<32>(C1->getZExtValue()))
+      N0 = Addr.getOperand(0);
+    else
+      C1 = nullptr;
+  }
 
-    if (N0.getOpcode() == ISD::ADD) {
-      // (add (add N2, N3), C1) -> addr64
-      SDValue N2 = N0.getOperand(0);
-      SDValue N3 = N0.getOperand(1);
-      Addr64 = CurDAG->getTargetConstant(1, DL, MVT::i1);
+    // (add N2, N3) -> addr64, or
+
+    if (N2->isDivergent()) {
+      if (N3->isDivergent()) {
+        // Both N2 and N3 are divergent. Use N0 (the result of the add) as the
+        // addr64, and construct the resource from a 0 address.
+        Ptr = SDValue(buildSMovImm64(DL, 0, MVT::v2i32), 0);
+        VAddr = N0;
+      } else {
+        // N2 is divergent, N3 is not.
+        Ptr = N3;
+        VAddr = N2;
+      }
+    } else {
+      // N2 is not divergent.
       Ptr = N2;
       VAddr = N3;
-    } else {
-      // (add N0, C1) -> offset
-      VAddr = CurDAG->getTargetConstant(0, DL, MVT::i32);
-      Ptr = N0;
-    }
+    }
+    Offset = CurDAG->getTargetConstant(0, DL, MVT::i16);
+    // N0 -> offset, or
+    // (N0 + C1) -> offset
 
-    if (SIInstrInfo::isLegalMUBUFImmOffset(C1->getZExtValue())) {
-      Offset = CurDAG->getTargetConstant(C1->getZExtValue(), DL, MVT::i16);
-      return true;
-    }
+  if (!C1) {
+    // No offset.
+    Offset = CurDAG->getTargetConstant(0, DL, MVT::i16);
+    return true;
+  }
+
+    // Legal offset for instruction.
 
-    if (isUInt<32>(C1->getZExtValue())) {
-      // Illegal offset, store it in soffset.
-      Offset = CurDAG->getTargetConstant(0, DL, MVT::i16);
-      SOffset = SDValue(CurDAG->getMachineNode(AMDGPU::S_MOV_B32, DL, MVT::i32,
-                   CurDAG->getTargetConstant(C1->getZExtValue(), DL, MVT::i32)),
-                        0);
-      return true;
-    }
-  }
-
-  if (Addr.getOpcode() == ISD::ADD) {
-    // (add N0, N1) -> addr64
-    SDValue N0 = Addr.getOperand(0);
-    SDValue N1 = Addr.getOperand(1);
-    Addr64 = CurDAG->getTargetConstant(1, DL, MVT::i1);
-    Ptr = N0;
-    VAddr = N1;
-    Offset = CurDAG->getTargetConstant(0, DL, MVT::i16);
-    return true;
-  }
-
-  // default case -> offset
-  VAddr = CurDAG->getTargetConstant(0, DL, MVT::i32);
-  Ptr = Addr;
-  Offset = CurDAG->getTargetConstant(0, DL, MVT::i16);
-
-  return true;
-}
+  SOffset =
+      SDValue(CurDAG->getMachineNode(
+                  AMDGPU::S_MOV_B32, DL, MVT::i32,
 
 bool AMDGPUDAGToDAGISel::SelectMUBUFAddr64(SDValue Addr, SDValue &SRsrc,
                                            SDValue &VAddr, SDValue &SOffset,
                                            SDValue &Offset, SDValue &GLC,
                                            SDValue &SLC, SDValue &TFE) const {
   SDValue Ptr, Offen, Idxen, Addr64;
 
   // addr64 bit was removed for volcanic islands.

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
+
+; Check that an addrspace(1) (const) load with various combinations of
+; uniform, nonuniform and constant address components all load with an
+; addr64 mubuf with no readfirstlane.
+
+@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>]
+
+; GCN-LABEL: {{^}}nonuniform_uniform:
+; GCN-NOT: readfirstlane
+; SICI: buffer_load_dwordx4 {{.*}} addr64
+
+define amdgpu_ps float @nonuniform_uniform(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
+}
+
+; GCN-LABEL: {{^}}uniform_nonuniform:
+; GCN-NOT: readfirstlane
+; SICI: buffer_load_dwordx4 {{.*}} addr64
+
+define amdgpu_ps float @uniform_nonuniform(i32 inreg %offset, i32 %arg18) {
+.entry:
+  %tmp1 = zext i32 %arg18 to i64
+  %tmp2 = inttoptr i64 %tmp1 to [6 x <3 x float>] addrspace(1)*
+  %tmp32 = getelementptr [6 x <3 x float>], [6 x <3 x float>] addrspace(1)* %tmp2, i32 0, i32 %offset
+  %tmp33 = load <3 x float>, <3 x float> addrspace(1)* %tmp32, align 16
+  %tmp34 = extractelement <3 x float> %tmp33, i32 0
+  ret float %tmp34
+}
+
+; GCN-LABEL: {{^}}nonuniform_nonuniform:
+; GCN-NOT: readfirstlane
+; SICI: buffer_load_dwordx4 {{.*}} addr64
+
+define amdgpu_ps float @nonuniform_nonuniform(i32 %offset, i32 %arg18) {
+.entry:
+  %tmp1 = zext i32 %arg18 to i64
+  %tmp2 = inttoptr i64 %tmp1 to [6 x <3 x float>] addrspace(1)*
+  %tmp32 = getelementptr [6 x <3 x float>], [6 x <3 x float>] addrspace(1)* %tmp2, i32 0, i32 %offset
+  %tmp33 = load <3 x float>, <3 x float> addrspace(1)* %tmp32, align 16
+  %tmp34 = extractelement <3 x float> %tmp33, i32 0
+  ret float %tmp34
+}
+
+; GCN-LABEL: {{^}}nonuniform_uniform_const:
+; GCN-NOT: readfirstlane
+; SICI: buffer_load_dword {{.*}} addr64
+
+define amdgpu_ps float @nonuniform_uniform_const(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, i64 1
+  %tmp33 = load float, float addrspace(1)* %tmp32, align 4
+  ret float %tmp33
+}
+
+; GCN-LABEL: {{^}}uniform_nonuniform_const:
+; GCN-NOT: readfirstlane
+; SICI: buffer_load_dword {{.*}} addr64
+
+define amdgpu_ps float @uniform_nonuniform_const(i32 inreg %offset, i32 %arg18) {
+.entry:
+  %tmp1 = zext i32 %arg18 to i64
+  %tmp2 = inttoptr i64 %tmp1 to [6 x <3 x float>] addrspace(1)*
+  %tmp32 = getelementptr [6 x <3 x float>], [6 x <3 x float>] addrspace(1)* %tmp2, i32 0, i32 %offset, i32 1
+  %tmp33 = load float, float addrspace(1)* %tmp32, align 4
+  ret float %tmp33
+}
+
+; GCN-LABEL: {{^}}nonuniform_nonuniform_const:
+; GCN-NOT: readfirstlane
+; SICI: buffer_load_dword {{.*}} addr64
+
+define amdgpu_ps float @nonuniform_nonuniform_const(i32 %offset, i32 %arg18) {
+.entry:
+  %tmp1 = zext i32 %arg18 to i64
+  %tmp2 = inttoptr i64 %tmp1 to [6 x <3 x float>] addrspace(1)*
+  %tmp32 = getelementptr [6 x <3 x float>], [6 x <3 x float>] addrspace(1)* %tmp2, i32 0, i32 %offset, i32 1
+  %tmp33 = load float, float addrspace(1)* %tmp32, align 4
+  ret float %tmp33
+}
+
+
+
+