CodeGen: Add a hook for getFenceOperandTy

[yaxunl]

Summary

Currently the operand type for ATOMIC_FENCE assumes value type of a pointer in address space 0.
This is fine for most targets. However for amdgcn target, the size of pointer in address space 0
depends on triple environment. For amdgiz environment, it is 64 bit but for other environment it is
32 bit. On the other hand, amdgcn target expects 32 bit fence operands independent of the target
triple environment. Therefore a hook is need in target lowering for getting the fence operand type.

This patch has no effect on targets other than amdgcn.

[arsenm]

Alternatively could these just be made i32 target constants for all targets?

lib/Target/AMDGPU/AMDGPUISelLowering.h

235 ↗

(On Diff #95620)

Extra space before (

[yaxunl]

In D32186#730742, @arsenm wrote:

Alternatively could these just be made i32 target constants for all targets?

It requires changes lowering of ATOMIC_FENCE in several backends. I am not sure if those backends like this change.

[kzhuravl]

include/llvm/Target/TargetLowering.h

242 ↗

(On Diff #95620)

new line.

lib/Target/AMDGPU/SIInstructions.td

97 ↗

(On Diff #95620)

InstSI -> SPseudoInstSI.

102–104 ↗

(On Diff #95620)

Remove these, as those are inherited from SPseudoInstSI.

[yaxunl]

Revised by Matt's and Konstantin's comments.

[t-tye]

In D32186#730749, @yaxunl wrote:

In D32186#730742, @arsenm wrote:

Alternatively could these just be made i32 target constants for all targets?

It requires changes lowering of ATOMIC_FENCE in several backends. I am not sure if those backends like this change.

I think making these constants makes more sense than inventing getFenceOperandTy(). It seems other targets are using the size of a pointer to address space 0 which does not seem right as these operands are for the memory scope and memory ordering which are not pointer types. Assuming that the size of a pointer to address space 0 is the size of size_t also seems incorrect. Since these values are in fact integer constants then doing as @arsenm suggests above seems the better approach.

However, that would require changing other backends, so this approach seems to be a work around to avoid that. So perhaps a TODO could be added that explains what the right fix is, and see if other backends would agree to the change in a later patch?

LGTM if add TODO.

[yaxunl]

Add TODO by Tony's comments.

Any further comments? Thanks.

llvm/trunk/include/llvm/Target/TargetLowering.h

public:
   }
 
   /// Return the type for frame index, which is determined by
   /// the alloca address space specified through the data layout.
   MVT getFrameIndexTy(const DataLayout &DL) const {
     return getPointerTy(DL, DL.getAllocaAddrSpace());
   }
 
+  /// Return the type for operands of fence.
+  /// TODO: Let fence operands be of i32 type and remove this.
+  virtual MVT getFenceOperandTy(const DataLayout &DL) const {
+    return getPointerTy(DL);
+  }
+
   /// EVT is not used in-tree, but is used by out-of-tree target.
   /// A documentation for this function would be nice...
   virtual MVT getScalarShiftAmountTy(const DataLayout &, EVT) const;
 
   EVT getShiftAmountTy(EVT LHSTy, const DataLayout &DL) const;
 
   /// Returns the type to be used for the index operand of:
   /// ISD::INSERT_VECTOR_ELT, ISD::EXTRACT_VECTOR_ELT,

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

 }
 
 void SelectionDAGBuilder::visitFence(const FenceInst &I) {
   SDLoc dl = getCurSDLoc();
   const TargetLowering &TLI = DAG.getTargetLoweringInfo();
   SDValue Ops[3];
   Ops[0] = getRoot();
   Ops[1] = DAG.getConstant((unsigned)I.getOrdering(), dl,
-                           TLI.getPointerTy(DAG.getDataLayout()));
+                           TLI.getFenceOperandTy(DAG.getDataLayout()));
   Ops[2] = DAG.getConstant(I.getSynchScope(), dl,
-                           TLI.getPointerTy(DAG.getDataLayout()));
+                           TLI.getFenceOperandTy(DAG.getDataLayout()));
   DAG.setRoot(DAG.getNode(ISD::ATOMIC_FENCE, dl, MVT::Other, Ops));
 }
 
 void SelectionDAGBuilder::visitAtomicLoad(const LoadInst &I) {
   SDLoc dl = getCurSDLoc();
   AtomicOrdering Order = I.getOrdering();
   SynchronizationScope Scope = I.getSynchScope();
 

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

public:
   /// \brief Helper function that returns the byte offset of the given
   /// type of implicit parameter.
   uint32_t getImplicitParameterOffset(const AMDGPUMachineFunction *MFI,
                                       const ImplicitParameter Param) const;
 
   AMDGPUAS getAMDGPUAS() const {
     return AMDGPUASI;
   }
+
+  MVT getFenceOperandTy(const DataLayout &DL) const override {
+    return MVT::i32;
+  }
 };
 
 namespace AMDGPUISD {
 
 enum NodeType : unsigned {
   // AMDIL ISD Opcodes
   FIRST_NUMBER = ISD::BUILTIN_OP_END,
   UMUL,        // 32bit unsigned multiplication

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

defm V_INTERP_MOV_F32 : VINTRP_m <
   [(set f32:$vdst, (AMDGPUinterp_mov (i32 imm:$vsrc), (i32 imm:$attrchan),
                                      (i32 imm:$attr)))]>;
 
 } // End Uses = [M0, EXEC]
 
 //===----------------------------------------------------------------------===//
 // Pseudo Instructions
 //===----------------------------------------------------------------------===//
+def ATOMIC_FENCE : SPseudoInstSI<
+  (outs), (ins i32imm:$ordering, i32imm:$scope),
+  [(atomic_fence (i32 imm:$ordering), (i32 imm:$scope))],
+  "ATOMIC_FENCE $ordering, $scope"> {
+  let hasSideEffects = 1;
+}
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 0, Uses = [EXEC] in {
 
 // For use in patterns
 def V_CNDMASK_B64_PSEUDO : VOP3Common <(outs VReg_64:$vdst),
   (ins VSrc_b64:$src0, VSrc_b64:$src1, SSrc_b64:$src2), "", []> {
   let isPseudo = 1;
   let isCodeGenOnly = 1;

llvm/trunk/test/CodeGen/AMDGPU/fence-amdgiz.ll

+; RUN: llc < %s | FileCheck %s
+
+target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:32:32-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-A5"
+target triple = "amdgcn-amd-amdhsa-amdgizcl"
+
+; CHECK_LABEL: atomic_fence
+; CHECK: BB#0:
+; CHECK: ATOMIC_FENCE 4, 1
+; CHECK: s_endpgm
+
+define amdgpu_kernel void @atomic_fence() {
+  fence acquire
+  ret void
+}
+