Add/Implement AddresSpace to PseudoSourceValue.

[jsjodin]

Summary

This is built on top of https://reviews.llvm.org/D21688 (not yet committed). Instead of taking the address space as an argument in the constructor we use a the target and callback inside the constructor since the source kind should have enough information for the target to decide which address space to use. This also contains the changes in the rest of the compiler to use the new interface. The TII argument does not have a default value (nullptr) because it should be explicit that the target is not needed and the generic address space will be used in all cases, otherwise it is easy to forget and could cause bugs that would be hard to find.

[arsenm]

lib/CodeGen/PseudoSourceValue.cpp

30

If it's required, TII should be a reference or at least this should assert

lib/Target/AMDGPU/SIInstrInfo.cpp

1771–1775

These should all be CONSTANT_ADDRESS

1777

Prefereable to use the enum value rather than 0

lib/Target/Mips/MipsMachineFunction.cpp

96 ↗

(On Diff #105530)

I thought the PSVManager held a TII reference?

[jsjodin]

lib/CodeGen/PseudoSourceValue.cpp

30

Yes, I think it may be better to have an assert. That way we don't have to dereference any pointers at the call sites. My initial idea was that if the target only has a generic address space there would be no need for TII, so a nullptr would be okay, but it would at least be explicit in the call, but that may be more confusing. I will fix this.

lib/Target/Mips/MipsMachineFunction.cpp

96 ↗

(On Diff #105530)

I wasn't sure, but in that case we can use TII, and put an assert in the PSV constructor.

[jsjodin]

Make TII mandatory (assert if given nullptr) in PSV constructor. Add TII as a member to the PSV manager so that it is not needed in the method calls.

[yaxunl]

Ping.

Any progress? D35361 relies on this patch and we need it to fix vload_private/vstore_private failure with new address space mapping.

[hfinkel]

Can you please upload a full-context patch?

[jsjodin]

Merged code with TOT and made full context patch.

[hfinkel]

lib/CodeGen/PseudoSourceValue.cpp

30

I don't understand this response. Can you make this a reference? It seems required here in the constructor.

lib/Target/AMDGPU/SIInstrInfo.h

261

Line is too long.

[jsjodin]

lib/CodeGen/PseudoSourceValue.cpp

30

Yes. it can be made into a reference. I will post a new patch with this.

[jsjodin]

Made changes to address comments by Hal Finkel.

[hfinkel]

LGTM

lib/Target/AMDGPU/SIInstrInfo.cpp

1848

Indentation off here.

include/llvm/CodeGen/PseudoSourceValue.h

 class MachineFrameInfo;
 class MachineMemOperand;
 class raw_ostream;
+class TargetInstrInfo;
 
 raw_ostream &operator<<(raw_ostream &OS, const MachineMemOperand &MMO);
 class PseudoSourceValue;

 
 private:
   PSVKind Kind;
+  unsigned AddressSpace;
   friend raw_ostream &llvm::operator<<(raw_ostream &OS,
                                        const PseudoSourceValue* PSV);
 

   virtual void printCustom(raw_ostream &O) const;
 
 public:
-  explicit PseudoSourceValue(PSVKind Kind);
+  explicit PseudoSourceValue(PSVKind Kind, const TargetInstrInfo *TII);
 
   virtual ~PseudoSourceValue();
 

   bool isGOT() const { return Kind == GOT; }
   bool isConstantPool() const { return Kind == ConstantPool; }
   bool isJumpTable() const { return Kind == JumpTable; }
+
+  unsigned getAddressSpace() const { return AddressSpace; }
+
   unsigned getTargetCustom() const {
     return (Kind >= TargetCustom) ? ((Kind+1) - TargetCustom) : 0;
   }

   const int FI;
 
 public:
-  explicit FixedStackPseudoSourceValue(int FI)
-      : PseudoSourceValue(FixedStack), FI(FI) {}
+  explicit FixedStackPseudoSourceValue(int FI, const TargetInstrInfo *TII)
+      : PseudoSourceValue(FixedStack, TII), FI(FI) {}
 
   static bool classof(const PseudoSourceValue *V) {
     return V->kind() == FixedStack;

 
 class CallEntryPseudoSourceValue : public PseudoSourceValue {
 protected:
-  CallEntryPseudoSourceValue(PSVKind Kind);
+  CallEntryPseudoSourceValue(PSVKind Kind, const TargetInstrInfo *TII);
 
 public:
   bool isConstant(const MachineFrameInfo *) const override;

   const GlobalValue *GV;
 
 public:
-  GlobalValuePseudoSourceValue(const GlobalValue *GV);
+  GlobalValuePseudoSourceValue(const GlobalValue *GV, const TargetInstrInfo *TII);
 
   static bool classof(const PseudoSourceValue *V) {
     return V->kind() == GlobalValueCallEntry;

   const char *ES;
 
 public:
-  ExternalSymbolPseudoSourceValue(const char *ES);
+  ExternalSymbolPseudoSourceValue(const char *ES, const TargetInstrInfo *TII);
 
   static bool classof(const PseudoSourceValue *V) {
     return V->kind() == ExternalSymbolCallEntry;

 
 /// Manages creation of pseudo source values.
 class PseudoSourceValueManager {
+  const TargetInstrInfo *TII;
   const PseudoSourceValue StackPSV, GOTPSV, JumpTablePSV, ConstantPoolPSV;
   std::map<int, std::unique_ptr<FixedStackPseudoSourceValue>> FSValues;
   StringMap<std::unique_ptr<const ExternalSymbolPseudoSourceValue>>

       GlobalCallEntries;
 
 public:
-  PseudoSourceValueManager();
+  PseudoSourceValueManager(const TargetInstrInfo *TII);
 
   /// Return a pseudo source value referencing the area below the stack frame of
   /// a function, e.g., the argument space.

include/llvm/Target/TargetInstrInfo.h

 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
 #include "llvm/CodeGen/MachineOperand.h"
+#include "llvm/CodeGen/PseudoSourceValue.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/Support/BranchProbability.h"
 #include "llvm/Support/ErrorHandling.h"

   }
 
 public:
+  /// getAddressSpaceForPseudoSourceKind - Given the kind of memory
+  /// (e.g. stack) the target returns the corresponding address space.
+  virtual unsigned
+  getAddressSpaceForPseudoSourceKind(PseudoSourceValue::PSVKind Kind) const {
+    return 0;
+  }
+
   /// unfoldMemoryOperand - Separate a single instruction which folded a load or
   /// a store or a load and a store into two or more instruction. If this is
   /// possible, returns true as well as the new instructions by reference.

lib/CodeGen/MachineFunction.cpp

          "Can't create a MachineFunction using a Module with a "
          "Target-incompatible DataLayout attached\n");
 
-  PSVManager = llvm::make_unique<PseudoSourceValueManager>();
+  PSVManager =
+    llvm::make_unique<PseudoSourceValueManager>(getSubtarget().getInstrInfo());
 }
 
 MachineFunction::~MachineFunction() {

lib/CodeGen/MachineInstr.cpp

 /// getAddrSpace - Return the LLVM IR address space number that this pointer
 /// points into.
 unsigned MachinePointerInfo::getAddrSpace() const {
-  if (V.isNull() || V.is<const PseudoSourceValue*>()) return 0;
+  if (V.isNull()) return 0;
+
+  if (V.is<const PseudoSourceValue*>())
+    return V.get<const PseudoSourceValue*>()->getAddressSpace();
+
   return cast<PointerType>(V.get<const Value*>()->getType())->getAddressSpace();
 }
 

lib/CodeGen/PseudoSourceValue.cpp

 #include "llvm/CodeGen/PseudoSourceValue.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/CodeGen/MachineFrameInfo.h"
+#include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/IR/DerivedTypes.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/Support/ErrorHandling.h"

     "Stack", "GOT", "JumpTable", "ConstantPool", "FixedStack",
     "GlobalValueCallEntry", "ExternalSymbolCallEntry"};
 
-PseudoSourceValue::PseudoSourceValue(PSVKind Kind) : Kind(Kind) {}
+PseudoSourceValue::PseudoSourceValue(PSVKind Kind, const TargetInstrInfo *TII)
+    : Kind(Kind) {
+  assert(TII != nullptr);
+  AddressSpace = TII->getAddressSpaceForPseudoSourceKind(Kind);
+}
+
 
 PseudoSourceValue::~PseudoSourceValue() {}
 

   OS << "FixedStack" << FI;
 }
 
-CallEntryPseudoSourceValue::CallEntryPseudoSourceValue(PSVKind Kind)
-    : PseudoSourceValue(Kind) {}
+CallEntryPseudoSourceValue::CallEntryPseudoSourceValue(
+    PSVKind Kind, const TargetInstrInfo *TII)
+    : PseudoSourceValue(Kind, TII) {}
 
 bool CallEntryPseudoSourceValue::isConstant(const MachineFrameInfo *) const {
   return false;

 }
 
 GlobalValuePseudoSourceValue::GlobalValuePseudoSourceValue(
-    const GlobalValue *GV)
-    : CallEntryPseudoSourceValue(GlobalValueCallEntry), GV(GV) {}
-
-ExternalSymbolPseudoSourceValue::ExternalSymbolPseudoSourceValue(const char *ES)
-    : CallEntryPseudoSourceValue(ExternalSymbolCallEntry), ES(ES) {}
-
-PseudoSourceValueManager::PseudoSourceValueManager()
-    : StackPSV(PseudoSourceValue::Stack), GOTPSV(PseudoSourceValue::GOT),
-      JumpTablePSV(PseudoSourceValue::JumpTable),
-      ConstantPoolPSV(PseudoSourceValue::ConstantPool) {}
+    const GlobalValue *GV,
+    const TargetInstrInfo *TII)
+    : CallEntryPseudoSourceValue(GlobalValueCallEntry, TII), GV(GV) {}
+ExternalSymbolPseudoSourceValue::ExternalSymbolPseudoSourceValue(
+    const char *ES, const TargetInstrInfo *TII)
+    : CallEntryPseudoSourceValue(ExternalSymbolCallEntry, TII), ES(ES) {}
+
+PseudoSourceValueManager::PseudoSourceValueManager(const TargetInstrInfo *TIInfo)
+    : TII(TIInfo),
+      StackPSV(PseudoSourceValue::Stack, TII),
+      GOTPSV(PseudoSourceValue::GOT, TII),
+      JumpTablePSV(PseudoSourceValue::JumpTable, TII),
+      ConstantPoolPSV(PseudoSourceValue::ConstantPool, TII) {}
 
 const PseudoSourceValue *PseudoSourceValueManager::getStack() {
   return &StackPSV;

   return &JumpTablePSV;
 }
 
-const PseudoSourceValue *PseudoSourceValueManager::getFixedStack(int FI) {
+const PseudoSourceValue *
+PseudoSourceValueManager::getFixedStack(int FI) {
   std::unique_ptr<FixedStackPseudoSourceValue> &V = FSValues[FI];
   if (!V)
-    V = llvm::make_unique<FixedStackPseudoSourceValue>(FI);
+    V = llvm::make_unique<FixedStackPseudoSourceValue>(FI, TII);
   return V.get();
 }
 

   std::unique_ptr<const GlobalValuePseudoSourceValue> &E =
       GlobalCallEntries[GV];
   if (!E)
-    E = llvm::make_unique<GlobalValuePseudoSourceValue>(GV);
+    E = llvm::make_unique<GlobalValuePseudoSourceValue>(GV, TII);
   return E.get();
 }
 

   std::unique_ptr<const ExternalSymbolPseudoSourceValue> &E =
       ExternalCallEntries[ES];
   if (!E)
-    E = llvm::make_unique<ExternalSymbolPseudoSourceValue>(ES);
+    E = llvm::make_unique<ExternalSymbolPseudoSourceValue>(ES, TII);
   return E.get();
 }

lib/Target/AMDGPU/SIInstrInfo.h

                           unsigned DstReg, ArrayRef<MachineOperand> Cond,
                           unsigned TrueReg, unsigned FalseReg) const;
 
+  unsigned getAddressSpaceForPseudoSourceKind(PseudoSourceValue::PSVKind Kind) const;
+
   bool
   areMemAccessesTriviallyDisjoint(MachineInstr &MIa, MachineInstr &MIb,
                                   AliasAnalysis *AA = nullptr) const override;

lib/Target/AMDGPU/SIInstrInfo.cpp

   }
 }
 
+unsigned SIInstrInfo::getAddressSpaceForPseudoSourceKind(
+    PseudoSourceValue::PSVKind Kind) const {
+  switch(Kind) {
+  case PseudoSourceValue::Stack:
+  case PseudoSourceValue::FixedStack:
+    return AMDGPUASI.PRIVATE_ADDRESS;
+  case PseudoSourceValue::ConstantPool:
+  case PseudoSourceValue::GOT:
+  case PseudoSourceValue::JumpTable:
+  case PseudoSourceValue::GlobalValueCallEntry:
+  case PseudoSourceValue::ExternalSymbolCallEntry:
+  case PseudoSourceValue::TargetCustom:
+    return AMDGPUASI.CONSTANT_ADDRESS;
+  default:
+      return AMDGPUASI.FLAT_ADDRESS;
+  }
+}
+
+
 static void removeModOperands(MachineInstr &MI) {
   unsigned Opc = MI.getOpcode();
   int Src0ModIdx = AMDGPU::getNamedOperandIdx(Opc,

lib/Target/AMDGPU/SIMachineFunctionInfo.h

 #include "MCTargetDesc/AMDGPUMCTargetDesc.h"
 #include "SIRegisterInfo.h"
 #include "llvm/CodeGen/PseudoSourceValue.h"
+#include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/Support/ErrorHandling.h"
 #include <array>

 
 class AMDGPUImagePseudoSourceValue : public PseudoSourceValue {
 public:
-  explicit AMDGPUImagePseudoSourceValue() :
-    PseudoSourceValue(PseudoSourceValue::TargetCustom) { }
+  explicit AMDGPUImagePseudoSourceValue(const TargetInstrInfo *TII) :
+    PseudoSourceValue(PseudoSourceValue::TargetCustom, TII) { }
 
   bool isConstant(const MachineFrameInfo *) const override {
     // This should probably be true for most images, but we will start by being

 
 class AMDGPUBufferPseudoSourceValue : public PseudoSourceValue {
 public:
-  explicit AMDGPUBufferPseudoSourceValue() :
-    PseudoSourceValue(PseudoSourceValue::TargetCustom) { }
+  explicit AMDGPUBufferPseudoSourceValue(const TargetInstrInfo *TII) :
+    PseudoSourceValue(PseudoSourceValue::TargetCustom, TII) { }
 
   bool isConstant(const MachineFrameInfo *) const override {
     // This should probably be true for most images, but we will start by being

lib/Target/AMDGPU/SIMachineFunctionInfo.cpp

     WavesPerEU(0, 0),
     DebuggerWorkGroupIDStackObjectIndices({{0, 0, 0}}),
     DebuggerWorkItemIDStackObjectIndices({{0, 0, 0}}),
+    BufferPSV(MF.getSubtarget().getInstrInfo()),
+    ImagePSV(MF.getSubtarget().getInstrInfo()),
     LDSWaveSpillSize(0),
     NumUserSGPRs(0),
     NumSystemSGPRs(0),