AMDGPU: Only use legal inline immediates with kill pseudo

[arsenm]

Summary

Only if the value is negative or positive is what matters,
so use a constant that doesn't require an instruction to
materialize.

[arsenm]

more tests

[nhaehnle]

I don't understand why changing the type to i32 is necessary. Isn't -1.0 also a legal inline immediate practically everywhere?

[arsenm]

In D22108#477764, @nhaehnle wrote:

I don't understand why changing the type to i32 is necessary. Isn't -1.0 also a legal inline immediate practically everywhere?

It's not, it's just more natural to express a boolean with an integer

[nhaehnle]

I agree, but the D3D designers in their infinite wisdom designed a KILL opcode with a floating point parameter, which we inherited via TGSI in Mesa, which in turn motivated this (indeed somewhat questionable) intrinsic.

Hmm. My concern is that (non-constant) KILL gets lowered to a floating point comparison, which treats NaNs specially. (i32)-1 is a NaN. I admit that it doesn't really matter since this patch uses a constant, but it still leaves me with an uncomfortable feeling.

[arsenm]

In D22108#478244, @nhaehnle wrote:

I agree, but the D3D designers in their infinite wisdom designed a KILL opcode with a floating point parameter, which we inherited via TGSI in Mesa, which in turn motivated this (indeed somewhat questionable) intrinsic.

Hmm. My concern is that (non-constant) KILL gets lowered to a floating point comparison, which treats NaNs specially. (i32)-1 is a NaN. I admit that it doesn't really matter since this patch uses a constant, but it still leaves me with an uncomfortable feeling.

The intrinsic itself's type doesn't change, this is just an internal implementation detail

[arsenm]

Use -1.0

[arsenm]

ping

[nhaehnle]

I forgot about this patch. I investigated our kill code generation some more, and it turns out that changing the "default" kill intrinsic to take an i1 instead of a float leads to better code anyway.

So if this change here is urgent to you for some reason, just go ahead and commit as-is and I'll rebase my changes on top of it, otherwise I'll soon post a change that should end up having the same effect.

[arsenm]

In D22108#488377, @nhaehnle wrote:

I forgot about this patch. I investigated our kill code generation some more, and it turns out that changing the "default" kill intrinsic to take an i1 instead of a float leads to better code anyway.

So if this change here is urgent to you for some reason, just go ahead and commit as-is and I'll rebase my changes on top of it, otherwise I'll soon post a change that should end up having the same effect.

Removing the kilp intrinsic would also be nice

[nhaehnle]

Yes, I'm working on a combination of patches for LLVM and Mesa that replaces both llvm.AMDGPU.kill and llvm.amdgpu.kilp with an llvm.amdgcn.kill(i1) intrinsic, where the argument is true if the thread should be killed.

[arsenm]

r275988

lib/Target/AMDGPU/AMDGPUISelLowering.h

enum NodeType : unsigned {
   BUILD_VERTICAL_VECTOR,
   /// Pointer to the start of the shader's constant data.
   CONST_DATA_PTR,
   SENDMSG,
   INTERP_MOV,
   INTERP_P1,
   INTERP_P2,
   PC_ADD_REL_OFFSET,
+  KILL,
   FIRST_MEM_OPCODE_NUMBER = ISD::FIRST_TARGET_MEMORY_OPCODE,
   STORE_MSKOR,
   LOAD_CONSTANT,
   TBUFFER_STORE_FORMAT,
   ATOMIC_CMP_SWAP,
   ATOMIC_INC,
   ATOMIC_DEC,
   LAST_AMDGPU_ISD_NUMBER
 };
 
 
 } // End namespace AMDGPUISD
 
 } // End namespace llvm
 
 #endif

lib/Target/AMDGPU/AMDGPUISelLowering.cpp

const char* AMDGPUTargetLowering::getTargetNodeName(unsigned Opcode) const {
   NODE_NAME_CASE(SAMPLEL)
   NODE_NAME_CASE(CVT_F32_UBYTE0)
   NODE_NAME_CASE(CVT_F32_UBYTE1)
   NODE_NAME_CASE(CVT_F32_UBYTE2)
   NODE_NAME_CASE(CVT_F32_UBYTE3)
   NODE_NAME_CASE(BUILD_VERTICAL_VECTOR)
   NODE_NAME_CASE(CONST_DATA_PTR)
   NODE_NAME_CASE(PC_ADD_REL_OFFSET)
+  NODE_NAME_CASE(KILL)
   case AMDGPUISD::FIRST_MEM_OPCODE_NUMBER: break;
   NODE_NAME_CASE(SENDMSG)
   NODE_NAME_CASE(INTERP_MOV)
   NODE_NAME_CASE(INTERP_P1)
   NODE_NAME_CASE(INTERP_P2)
   NODE_NAME_CASE(STORE_MSKOR)
   NODE_NAME_CASE(LOAD_CONSTANT)
   NODE_NAME_CASE(TBUFFER_STORE_FORMAT)

lib/Target/AMDGPU/AMDGPUInstrInfo.td

def AMDGPUDivScaleOp : SDTypeProfile<2, 3,
   [SDTCisFP<0>, SDTCisInt<1>, SDTCisSameAs<0, 2>, SDTCisSameAs<0, 3>, SDTCisSameAs<0, 4>]
 >;
 
 // float, float, float, vcc
 def AMDGPUFmasOp : SDTypeProfile<1, 4,
   [SDTCisFP<0>, SDTCisSameAs<0, 1>, SDTCisSameAs<0, 2>, SDTCisSameAs<0, 3>, SDTCisInt<4>]
 >;
 
+def AMDGPUKillSDT : SDTypeProfile<0, 1, [SDTCisInt<0>]>;
+
 //===----------------------------------------------------------------------===//
 // AMDGPU DAG Nodes
 //
 
 def AMDGPUconstdata_ptr : SDNode<
   "AMDGPUISD::CONST_DATA_PTR", SDTypeProfile <1, 1, [SDTCisVT<0, iPTR>,
                                                      SDTCisVT<0, iPTR>]>
 >;

 def AMDGPUinterp_p1 : SDNode<"AMDGPUISD::INTERP_P1",
                       SDTypeProfile<1, 3, [SDTCisFP<0>]>,
                       [SDNPInGlue, SDNPOutGlue]>;
 
 def AMDGPUinterp_p2 : SDNode<"AMDGPUISD::INTERP_P2",
                       SDTypeProfile<1, 4, [SDTCisFP<0>]>,
                       [SDNPInGlue]>;
 
+def AMDGPUkill : SDNode<"AMDGPUISD::KILL", AMDGPUKillSDT,
+  [SDNPHasChain, SDNPSideEffect]>;
+
 //===----------------------------------------------------------------------===//
 // Flow Control Profile Types
 //===----------------------------------------------------------------------===//
 // Branch instruction where second and third are basic blocks
 def SDTIL_BRCond : SDTypeProfile<0, 2, [
     SDTCisVT<0, OtherVT>
     ]>;
 

lib/Target/AMDGPU/SIISelLowering.cpp

case AMDGPUIntrinsic::SI_tbuffer_store: {
     MachineMemOperand *MMO = MF.getMachineMemOperand(
       MachinePointerInfo(),
       MachineMemOperand::MOStore,
       VT.getStoreSize(), 4);
     return DAG.getMemIntrinsicNode(AMDGPUISD::TBUFFER_STORE_FORMAT, DL,
                                    Op->getVTList(), Ops, VT, MMO);
   }
   case AMDGPUIntrinsic::AMDGPU_kill: {
-    if (const ConstantFPSDNode *K = dyn_cast<ConstantFPSDNode>(Op.getOperand(2))) {
+    SDValue Src = Op.getOperand(2);
+    if (const ConstantFPSDNode *K = dyn_cast<ConstantFPSDNode>(Src)) {
       if (!K->isNegative())
         return Chain;
+
+      SDValue NegOne = DAG.getTargetConstant(FloatToBits(-1.0f), DL, MVT::i32);
+      return DAG.getNode(AMDGPUISD::KILL, DL, MVT::Other, Chain, NegOne);
     }
 
-    return Op;
+    SDValue Cast = DAG.getNode(ISD::BITCAST, DL, MVT::i32, Src);
+    return DAG.getNode(AMDGPUISD::KILL, DL, MVT::Other, Chain, Cast);
   }
   default:
     return SDValue();
   }
 }
 
 SDValue SITargetLowering::LowerLOAD(SDValue Op, SelectionDAG &DAG) const {
   SDLoc DL(Op);

lib/Target/AMDGPU/SIInstructions.td

def SI_END_CF : PseudoInstSI <
   [(int_amdgcn_end_cf i64:$saved)]
 >;
 
 } // End Uses = [EXEC], Defs = [EXEC, SCC]
 
 let Uses = [EXEC], Defs = [EXEC,VCC] in {
 def SI_KILL : PseudoInstSI <
   (outs), (ins VSrc_32:$src),
-  [(int_AMDGPU_kill f32:$src)]> {
+  [(AMDGPUkill i32:$src)]> {
   let isConvergent = 1;
   let usesCustomInserter = 1;
 }
 
 def SI_KILL_TERMINATOR : PseudoInstSI <
   (outs), (ins VSrc_32:$src)> {
   let isTerminator = 1;
 }