AMDGPU: Move isSDNodeSourceOfDivergence() implementation to SITargetLowering

[tstellar]

Summary

The code that handles ISD:Register and ISD::CopyFromReg assumes
the target is amdgcn, so this is broken on r600. We don't
need this analysis on r600 anyway so we can safely move
it to SITargetLowering.

[msearles]

lib/Target/AMDGPU/AMDGPUISelLowering.cpp

811 ↗

(On Diff #144669)

Is the "// workitem.id.x workitem.id.y workitem.id.z" comment still relevant?

[alex-t]

Could you please clarify - why do you consider that check meaningless for r600?
I see that this line : " const SISubtarget &ST = MF->getSubtarget<SISubtarget>(); " is misleading and in fact is not correct.
I'd better check and choose the R600Subtarget or SISubtarget.
If I understand right we need just check which subtarget to retrieve for physregs check.

Since we consider any VGPR formal argument as divergent it does not matter R600 or SI at all.
We need to choose right TargetRegisterInfo (r600 or SI again)

So, for virtual register : if (MRI.isLiveIn(Reg) && TRI.isVGPR(Reg) ) return true

Or I maybe don't know something substantial? :)

[tstellar]

In D46298#1086262, @alex-t wrote:

Could you please clarify - why do you consider that check meaningless for r600?
I see that this line : " const SISubtarget &ST = MF->getSubtarget<SISubtarget>(); " is misleading and in fact is not correct.
I'd better check and choose the R600Subtarget or SISubtarget.
If I understand right we need just check which subtarget to retrieve for physregs check.

Since we consider any VGPR formal argument as divergent it does not matter R600 or SI at all.
We need to choose right TargetRegisterInfo (r600 or SI again)

So, for virtual register : if (MRI.isLiveIn(Reg) && TRI.isVGPR(Reg) ) return true

Or I maybe don't know something substantial? :)

I would expect the TRI.isVGPR() call to crash if this was called from an R600 code path, so that made me think this was dead code.

[arsenm]

In D46298#1110597, @tstellar wrote:

In D46298#1086262, @alex-t wrote:

Could you please clarify - why do you consider that check meaningless for r600?
I see that this line : " const SISubtarget &ST = MF->getSubtarget<SISubtarget>(); " is misleading and in fact is not correct.
I'd better check and choose the R600Subtarget or SISubtarget.
If I understand right we need just check which subtarget to retrieve for physregs check.

Since we consider any VGPR formal argument as divergent it does not matter R600 or SI at all.
We need to choose right TargetRegisterInfo (r600 or SI again)

So, for virtual register : if (MRI.isLiveIn(Reg) && TRI.isVGPR(Reg) ) return true

Or I maybe don't know something substantial? :)

I would expect the TRI.isVGPR() call to crash if this was called from an R600 code path, so that made me think this was dead code.

I agree isVGPR should be illegal to call for R600

[alex-t]

I agree isVGPR should be illegal to call for R600

So, how would you handle the bunch of r600 tests then?
We could claim the support of DA related stuff for GCN only but it should be explicitly agreed and documented somehow.

[arsenm]

In D46298#1110788, @alex-t wrote:

I agree isVGPR should be illegal to call for R600

So, how would you handle the bunch of r600 tests then?
We could claim the support of DA related stuff for GCN only but it should be explicitly agreed and documented somehow.

Disable this for r600. It doesn't have the SGPR/VGPR split issue we want this for, so it doesn't really matter

[tstellar]

In D46298#1110789, @arsenm wrote:

In D46298#1110788, @alex-t wrote:

I agree isVGPR should be illegal to call for R600

So, how would you handle the bunch of r600 tests then?
We could claim the support of DA related stuff for GCN only but it should be explicitly agreed and documented somehow.

Disable this for r600. It doesn't have the SGPR/VGPR split issue we want this for, so it doesn't really matter

My patch does this, but just for ISD:Register and ISD::CopyFromRegister, should I just disable this analysis completely for r600?

[alex-t]

should I just disable this analysis completely for r600?

I'd like this better. It looks like cleaner way.
And yes it's okay to remove r600 registers from isSDNodeSourceOfDivergence

[tstellar]

Completely disable isSDNodeSourceOfDivergence() for r600.

[tstellar]

Ping.

[arsenm]

lib/Target/AMDGPU/AMDGPUISelLowering.cpp

785–790 ↗

(On Diff #149790)

Just move this into SIISlLowering instead?

[tstellar]

Move isSDNodeSourceOfDivergence() implementation from AMDGPUTargetLowering
to SITargetLowering.

[arsenm]

LGTM

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

public:
 
   bool storeOfVectorConstantIsCheap(EVT MemVT,
                                     unsigned NumElem,
                                     unsigned AS) const override;
   bool aggressivelyPreferBuildVectorSources(EVT VecVT) const override;
   bool isCheapToSpeculateCttz() const override;
   bool isCheapToSpeculateCtlz() const override;
 
-  bool isSDNodeSourceOfDivergence(const SDNode *N,
-    FunctionLoweringInfo *FLI, DivergenceAnalysis *DA) const override;
   bool isSDNodeAlwaysUniform(const SDNode *N) const override;
   static CCAssignFn *CCAssignFnForCall(CallingConv::ID CC, bool IsVarArg);
   static CCAssignFn *CCAssignFnForReturn(CallingConv::ID CC, bool IsVarArg);
 
   SDValue LowerReturn(SDValue Chain, CallingConv::ID CallConv, bool isVarArg,
                       const SmallVectorImpl<ISD::OutputArg> &Outs,
                       const SmallVectorImpl<SDValue> &OutVals, const SDLoc &DL,
                       SelectionDAG &DAG) const override;

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

case ISD::LOAD:
       == Subtarget->getAMDGPUAS().CONSTANT_ADDRESS_32BIT)
         return true;
       return false;
     }
     break;
   }
 }
 
-bool AMDGPUTargetLowering::isSDNodeSourceOfDivergence(const SDNode * N,
-  FunctionLoweringInfo * FLI, DivergenceAnalysis * DA) const
-{
-  switch (N->getOpcode()) {
-    case ISD::Register:
-    case ISD::CopyFromReg:
-    {
-      const RegisterSDNode *R = nullptr;
-      if (N->getOpcode() == ISD::Register) {
-        R = dyn_cast<RegisterSDNode>(N);
-      }
-      else {
-        R = dyn_cast<RegisterSDNode>(N->getOperand(1));
-      }
-      if (R)
-      {
-        const MachineFunction * MF = FLI->MF;
-        const SISubtarget &ST = MF->getSubtarget<SISubtarget>();
-        const MachineRegisterInfo &MRI = MF->getRegInfo();
-        const SIRegisterInfo &TRI = ST.getInstrInfo()->getRegisterInfo();
-        unsigned Reg = R->getReg();
-        if (TRI.isPhysicalRegister(Reg))
-          return TRI.isVGPR(MRI, Reg);
-
-        if (MRI.isLiveIn(Reg)) {
-          // workitem.id.x workitem.id.y workitem.id.z
-          // Any VGPR formal argument is also considered divergent
-          if ((MRI.getLiveInPhysReg(Reg) == AMDGPU::T0_X) ||
-              (MRI.getLiveInPhysReg(Reg) == AMDGPU::T0_Y) ||
-              (MRI.getLiveInPhysReg(Reg) == AMDGPU::T0_Z) ||
-              (TRI.isVGPR(MRI, Reg)))
-              return true;
-          // Formal arguments of non-entry functions
-          // are conservatively considered divergent
-          else if (!AMDGPU::isEntryFunctionCC(FLI->Fn->getCallingConv()))
-            return true;
-        }
-        return !DA || DA->isDivergent(FLI->getValueFromVirtualReg(Reg));
-      }
-    }
-    break;
-    case ISD::LOAD: {
-      const LoadSDNode *L = dyn_cast<LoadSDNode>(N);
-      if (L->getMemOperand()->getAddrSpace() ==
-          Subtarget->getAMDGPUAS().PRIVATE_ADDRESS)
-        return true;
-    } break;
-    case ISD::CALLSEQ_END:
-    return true;
-    break;
-    case ISD::INTRINSIC_WO_CHAIN:
-    {
-
-    }
-      return AMDGPU::isIntrinsicSourceOfDivergence(
-      cast<ConstantSDNode>(N->getOperand(0))->getZExtValue());
-    case ISD::INTRINSIC_W_CHAIN:
-      return AMDGPU::isIntrinsicSourceOfDivergence(
-      cast<ConstantSDNode>(N->getOperand(1))->getZExtValue());
-    // In some cases intrinsics that are a source of divergence have been
-    // lowered to AMDGPUISD so we also need to check those too.
-    case AMDGPUISD::INTERP_MOV:
-    case AMDGPUISD::INTERP_P1:
-    case AMDGPUISD::INTERP_P2:
-      return true;
-  }
-  return false;
-}
-
 //===---------------------------------------------------------------------===//
 // Target Properties
 //===---------------------------------------------------------------------===//
 
 bool AMDGPUTargetLowering::isFAbsFree(EVT VT) const {
   assert(VT.isFloatingPoint());
 
   // Packed operations do not have a fabs modifier.

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

public:
 
   void finalizeLowering(MachineFunction &MF) const override;
 
   void computeKnownBitsForFrameIndex(const SDValue Op,
                                      KnownBits &Known,
                                      const APInt &DemandedElts,
                                      const SelectionDAG &DAG,
                                      unsigned Depth = 0) const override;
+
+  bool isSDNodeSourceOfDivergence(const SDNode *N,
+    FunctionLoweringInfo *FLI, DivergenceAnalysis *DA) const override;
 };
 
 } // End namespace llvm
 
 #endif

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

if (getSubtarget()->enableHugePrivateBuffer())
     return;
 
   // Technically it may be possible to have a dispatch with a single workitem
   // that uses the full private memory size, but that's not really useful. We
   // can't use vaddr in MUBUF instructions if we don't know the address
   // calculation won't overflow, so assume the sign bit is never set.
   Known.Zero.setHighBits(AssumeFrameIndexHighZeroBits);
 }
+
+bool SITargetLowering::isSDNodeSourceOfDivergence(const SDNode * N,
+  FunctionLoweringInfo * FLI, DivergenceAnalysis * DA) const
+{
+  switch (N->getOpcode()) {
+    case ISD::Register:
+    case ISD::CopyFromReg:
+    {
+      const RegisterSDNode *R = nullptr;
+      if (N->getOpcode() == ISD::Register) {
+        R = dyn_cast<RegisterSDNode>(N);
+      }
+      else {
+        R = dyn_cast<RegisterSDNode>(N->getOperand(1));
+      }
+      if (R)
+      {
+        const MachineFunction * MF = FLI->MF;
+        const SISubtarget &ST = MF->getSubtarget<SISubtarget>();
+        const MachineRegisterInfo &MRI = MF->getRegInfo();
+        const SIRegisterInfo &TRI = ST.getInstrInfo()->getRegisterInfo();
+        unsigned Reg = R->getReg();
+        if (TRI.isPhysicalRegister(Reg))
+          return TRI.isVGPR(MRI, Reg);
+
+        if (MRI.isLiveIn(Reg)) {
+          // workitem.id.x workitem.id.y workitem.id.z
+          // Any VGPR formal argument is also considered divergent
+          if (TRI.isVGPR(MRI, Reg))
+              return true;
+          // Formal arguments of non-entry functions
+          // are conservatively considered divergent
+          else if (!AMDGPU::isEntryFunctionCC(FLI->Fn->getCallingConv()))
+            return true;
+        }
+        return !DA || DA->isDivergent(FLI->getValueFromVirtualReg(Reg));
+      }
+    }
+    break;
+    case ISD::LOAD: {
+      const LoadSDNode *L = dyn_cast<LoadSDNode>(N);
+      if (L->getMemOperand()->getAddrSpace() ==
+          Subtarget->getAMDGPUAS().PRIVATE_ADDRESS)
+        return true;
+    } break;
+    case ISD::CALLSEQ_END:
+    return true;
+    break;
+    case ISD::INTRINSIC_WO_CHAIN:
+    {
+
+    }
+      return AMDGPU::isIntrinsicSourceOfDivergence(
+      cast<ConstantSDNode>(N->getOperand(0))->getZExtValue());
+    case ISD::INTRINSIC_W_CHAIN:
+      return AMDGPU::isIntrinsicSourceOfDivergence(
+      cast<ConstantSDNode>(N->getOperand(1))->getZExtValue());
+    // In some cases intrinsics that are a source of divergence have been
+    // lowered to AMDGPUISD so we also need to check those too.
+    case AMDGPUISD::INTERP_MOV:
+    case AMDGPUISD::INTERP_P1:
+    case AMDGPUISD::INTERP_P2:
+      return true;
+  }
+  return false;
+}