[AMDGPU] Lower null pointers in static variable initializer

[yaxunl]

Summary

For amdgcn target Clang generates addrspacecast to represent null pointers in private and local address spaces.

In LLVM codegen, the static variable initializer is lowered by virtual function AsmPrinter::lowerConstant which is target generic. Since addrspacecast is target specific, AsmPrinter::lowerConstant is unable to lower addrspacecast in the null pointer representation.

This patch overrides AsmPrinter::lowerConstant with AMDGPUAsmPrinter::lowerConstant, which is able to lower the target-specific addrspacecast in the null pointer representation so that -1 is correctly emitted in codeobject as initial value for null pointers.

[arsenm]

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

162–168

We should factor the constant folding of pointers into a single place. The same logic is already repeated in SIISelLowering and I have a need of it in another place.

Maybe we should put a constant fold pointer helper in AMDGPUTargetMachine?

[yaxunl]

Refactored constant folding of non-zero null pointer to a separate file.

[arsenm]

lib/Target/AMDGPU/Utils/AMDGPUConstantFoldUtils.cpp

17 ↗

(On Diff #86648)

This doesn't need to be its own file and isn't the right API to be re-used. I think the right place is in AMDGPUTargetMachine because this is logically part of the DataLayout.

The base function should just be the address spaces so it can be used in the DAG and MI

[arsenm]

Better test name might be nullptr-constant-initializer

test/CodeGen/AMDGPU/nullptr.ll

12

Should include tests for the other address spaces too

[yaxunl]

lib/Target/AMDGPU/Utils/AMDGPUConstantFoldUtils.cpp

17 ↗

(On Diff #86648)

I took a look of TargetMachine and its derived classes, I did not see a precedence to put such information there. Is it the right place to hold such information?

Another question. I am not quite sure about the desired API for this function. Do you want something like this?

uint64_t getNullPointerValue(unsigned AddrSpace);

which returns -1 for private/local address spaces and 0 for other address spaces?

[arsenm]

lib/Target/AMDGPU/Utils/AMDGPUConstantFoldUtils.cpp

17 ↗

(On Diff #86648)

I think so, it's more datalayout/constant related. e.g. getNameWithPrefix.

I think some of this handling is incorrectly in TargetLowering, e.g. D20758.

Something like that seems right

[yaxunl]

Added getNullPtrValue to TargetMachine and more tests.

[yaxunl]

Keep getNullPointerValue to AMDGPUTargetMachine only.

[arsenm]

LGTM except for minor detail

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

225–233

Body of the function should go in the header

228

Might as well handle REGION as well

[yaxunl]

Thanks. Will fix it when submitting.

lib/Target/AMDGPU/AMDGPUAsmPrinter.h

public:
   bool runOnMachineFunction(MachineFunction &MF) override;
 
   StringRef getPassName() const override;
 
   /// \brief Wrapper for MCInstLowering.lowerOperand() for the tblgen'erated
   /// pseudo lowering.
   bool lowerOperand(const MachineOperand &MO, MCOperand &MCOp) const;
 
+  /// \brief Lower the specified LLVM Constant to an MCExpr.
+  /// The AsmPrinter::lowerConstantof does not know how to lower
+  /// addrspacecast, therefore they should be lowered by this function.
+  const MCExpr *lowerConstant(const Constant *CV) override;
+
   /// \brief tblgen'erated driver function for lowering simple MI->MC pseudo
   /// instructions.
   bool emitPseudoExpansionLowering(MCStreamer &OutStreamer,
                                    const MachineInstr *MI);
 
   /// Implemented in AMDGPUMCInstLower.cpp
   void EmitInstruction(const MachineInstr *MI) override;
 

lib/Target/AMDGPU/AMDGPUMCInstLower.cpp

 
 bool AMDGPUAsmPrinter::lowerOperand(const MachineOperand &MO,
                                     MCOperand &MCOp) const {
   const AMDGPUSubtarget &STI = MF->getSubtarget<AMDGPUSubtarget>();
   AMDGPUMCInstLower MCInstLowering(OutContext, STI, *this);
   return MCInstLowering.lowerOperand(MO, MCOp);
 }
 
+const MCExpr *AMDGPUAsmPrinter::lowerConstant(const Constant *CV) {
+  // TargetMachine does not support llvm-style cast. Use C++-style cast.
+  // This is safe since TM is always of type AMDGPUTargetMachine or its
+  // derived class.
+  auto *AT = static_cast<AMDGPUTargetMachine*>(&TM);
+  auto *CE = dyn_cast<ConstantExpr>(CV);
+
+  // Lower null pointers in private and local address space.
+  // Clang generates addrspacecast for null pointers in private and local
+  // address space, which needs to be lowered.
+  if (CE && CE->getOpcode() == Instruction::AddrSpaceCast) {
+    auto Op = CE->getOperand(0);
+    auto SrcAddr = Op->getType()->getPointerAddressSpace();
+    if (Op->isNullValue() && AT->getNullPointerValue(SrcAddr) == 0) {
+      auto DstAddr = CE->getType()->getPointerAddressSpace();
+      return MCConstantExpr::create(AT->getNullPointerValue(DstAddr),
+        OutContext);
+    }
+  }
+  return AsmPrinter::lowerConstant(CV);
+}
+
 void AMDGPUAsmPrinter::EmitInstruction(const MachineInstr *MI) {
   if (emitPseudoExpansionLowering(*OutStreamer, MI))
     return;
 
   const AMDGPUSubtarget &STI = MF->getSubtarget<AMDGPUSubtarget>();
   AMDGPUMCInstLower MCInstLowering(OutContext, STI, *this);
 
   StringRef Err;

lib/Target/AMDGPU/AMDGPUTargetMachine.h

public:
   }
   TargetIRAnalysis getTargetIRAnalysis() override;
 
   TargetLoweringObjectFile *getObjFileLowering() const override {
     return TLOF.get();
   }
   void addPreLinkPasses(PassManagerBase & PM) override;
   void addEarlyAsPossiblePasses(PassManagerBase &PM) override;
+
+  /// Get the integer value of a null pointer in the given address space.
+  uint64_t getNullPointerValue(unsigned AddrSpace) const;
 };
 
 //===----------------------------------------------------------------------===//
 // R600 Target Machine (R600 -> Cayman)
 //===----------------------------------------------------------------------===//
 
 class R600TargetMachine final : public AMDGPUTargetMachine {
 private:

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp

void AMDGPUTargetMachine::addPreLinkPasses(PassManagerBase & PM) {
   PM.add(llvm::createAMDGPUPrintfRuntimeBinding());
   PM.add(llvm::createAMDGPUclpVectorExpansionPass());
 }
 
 void AMDGPUTargetMachine::addEarlyAsPossiblePasses(PassManagerBase &PM) {
   PM.add(createAMDGPUUnifyMetadataPass());
 }
 
+uint64_t AMDGPUTargetMachine::getNullPointerValue(unsigned AddrSpace) const {
+  switch(AddrSpace) {
+  case AMDGPUAS::PRIVATE_ADDRESS:
+  case AMDGPUAS::LOCAL_ADDRESS:
+    return -1;
+  default:
+    return 0;
+  }
+}
+
 //===----------------------------------------------------------------------===//
 // R600 Target Machine (R600 -> Cayman)
 //===----------------------------------------------------------------------===//
 
 R600TargetMachine::R600TargetMachine(const Target &T, const Triple &TT,
                                      StringRef CPU, StringRef FS,
                                      TargetOptions Options,
                                      Optional<Reloc::Model> RM,

test/CodeGen/AMDGPU/nullptr.ll

+;RUN: llc < %s -march=amdgcn -verify-machineinstrs | FileCheck %s
+
+%struct.S = type { i32*, i32 addrspace(1)*, i32 addrspace(2)*, i32 addrspace(3)*, i32 addrspace(4)*, i32 addrspace(5)*}
+
+; CHECK-LABEL: nullptr_priv:
+; CHECK-NEXT: .long -1
+@nullptr_priv = global i32* addrspacecast (i32 addrspace(4)* null to i32*)
+
+; CHECK-LABEL: nullptr_glob:
+; CHECK-NEXT: .quad 0
+@nullptr_glob = global i32 addrspace(1)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(1)*)
+
+; CHECK-LABEL: nullptr_const:
+; CHECK-NEXT: .quad 0
+@nullptr_const = global i32 addrspace(2)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(2)*)
+
+; CHECK-LABEL: nullptr_local:
+; CHECK-NEXT: .long -1
+@nullptr_local = global i32 addrspace(3)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(3)*)
+
+; CHECK-LABEL: nullptr_region:
+; CHECK-NEXT: .long 0
+@nullptr_region = global i32 addrspace(5)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(5)*)
+
+; CHECK-LABEL: nullptr6:
+; CHECK-NEXT: .long 0
+@nullptr6 = global i32 addrspace(6)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(6)*)
+
+; CHECK-LABEL: nullptr7:
+; CHECK-NEXT: .long 0
+@nullptr7 = global i32 addrspace(7)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(7)*)
+
+; CHECK-LABEL: nullptr8:
+; CHECK-NEXT: .long 0
+@nullptr8 = global i32 addrspace(8)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(8)*)
+
+; CHECK-LABEL: nullptr9:
+; CHECK-NEXT: .long 0
+@nullptr9 = global i32 addrspace(9)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(9)*)
+
+; CHECK-LABEL: nullptr10:
+; CHECK-NEXT: .long 0
+@nullptr10 = global i32 addrspace(10)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(10)*)
+
+; CHECK-LABEL: nullptr11:
+; CHECK-NEXT: .long 0
+@nullptr11 = global i32 addrspace(11)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(11)*)
+
+; CHECK-LABEL: nullptr12:
+; CHECK-NEXT: .long 0
+@nullptr12 = global i32 addrspace(12)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(12)*)
+
+; CHECK-LABEL: nullptr13:
+; CHECK-NEXT: .long 0
+@nullptr13 = global i32 addrspace(13)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(13)*)
+
+; CHECK-LABEL: nullptr14:
+; CHECK-NEXT: .long 0
+@nullptr14 = global i32 addrspace(14)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(14)*)
+
+; CHECK-LABEL: nullptr15:
+; CHECK-NEXT: .long 0
+@nullptr15 = global i32 addrspace(15)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(15)*)
+
+; CHECK-LABEL: nullptr16:
+; CHECK-NEXT: .long 0
+@nullptr16 = global i32 addrspace(16)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(16)*)
+
+; CHECK-LABEL: nullptr17:
+; CHECK-NEXT: .long 0
+@nullptr17 = global i32 addrspace(17)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(17)*)
+
+; CHECK-LABEL: nullptr18:
+; CHECK-NEXT: .long 0
+@nullptr18 = global i32 addrspace(18)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(18)*)
+
+; CHECK-LABEL: nullptr19:
+; CHECK-NEXT: .long 0
+@nullptr19 = global i32 addrspace(19)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(19)*)
+
+; CHECK-LABEL: nullptr20:
+; CHECK-NEXT: .long 0
+@nullptr20 = global i32 addrspace(20)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(20)*)
+
+; CHECK-LABEL: nullptr21:
+; CHECK-NEXT: .long 0
+@nullptr21 = global i32 addrspace(21)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(21)*)
+
+; CHECK-LABEL: nullptr22:
+; CHECK-NEXT: .long 0
+@nullptr22 = global i32 addrspace(22)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(22)*)
+
+; CHECK-LABEL: nullptr23:
+; CHECK-NEXT: .long 0
+@nullptr23 = global i32 addrspace(23)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(23)*)
+
+; CHECK-LABEL: structWithPointers:
+; CHECK-NEXT: .long -1
+; CHECK-NEXT: .zero 4
+; CHECK-NEXT: .quad 0
+; CHECK-NEXT: .quad 0
+; CHECK-NEXT: .long -1
+; CHECK-NEXT: .zero 4
+; CHECK-NEXT: .quad 0
+; CHECK-NEXT: .long 0
+; CHECK-NEXT: .zero 4
+@structWithPointers = addrspace(1) global %struct.S {
+  i32* addrspacecast (i32 addrspace(4)* null to i32*),
+  i32 addrspace(1)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(1)*),
+  i32 addrspace(2)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(2)*),
+  i32 addrspace(3)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(3)*),
+  i32 addrspace(4)* null,
+  i32 addrspace(5)* addrspacecast (i32 addrspace(4)* null to i32 addrspace(5)*)}, align 4