[PATCH] D31284: [AMDGPU] Get address space mapping by target triple

[Tony Tye via Phabricator via llvm-commits]

t-tye added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPU.h:181-209
+  const static unsigned GLOBAL_ADDRESS   = 1;  ///< Address space for global memory (RAT0, VTX0).
+  const static unsigned LOCAL_ADDRESS    = 3;  ///< Address space for local memory.
+  const static unsigned PARAM_D_ADDRESS  = 6;  ///< Address space for direct addressible parameter memory (CONST0)
+  const static unsigned PARAM_I_ADDRESS  = 7;  ///< Address space for indirect addressible parameter memory (VTX1)
 
   // Do not re-order the CONSTANT_BUFFER_* enums.  Several places depend on this
   // order to be able to dynamically index a constant buffer, for example:
----------------
t-tye wrote:
> Since all these are still constants, and the plan is to revert back to having an enumeration for AddressSpaces, would it be better to leave these as an anonymous enum enumerators and avoid the need for changing this text, or defining the large number of static variables needed?
Since you have already done the change probably best to leave as is for consistency. Note the few places that did not get updated to access these via . member selection.


================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp:51-52
+    unsigned AS2) {
   // This array is indexed by the AMDGPUAS::AddressSpaces
   // enum elements PRIVATE_ADDRESS ... to FLAT_ADDRESS
   // see "llvm/Transforms/AMDSPIRUtils.h"
----------------
I think this comment needs updating?


================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp:63-64
   };
+  auto Index1 = ASToIndex[AS1];
+  auto Index2 = ASToIndex[AS2];
+  assert(0 <= Index1 && Index1 < 5 && 0 <= Index2 && Index2 < 5);
----------------
Is using a DenseMap the best choice here? If an address space that is not one of the ones assigned in the ASAliasRulesTy constructor is used to index then won't it create a new entry and default construct the mapping meaning 0 will be returned?

Also DenseMap seems non-trivial, would if be better to initialize the ASAliasRules array so it can be indexed directly with the address space number (asserting that the 5 address spaces we care about are <5).


================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp:73-79
+  assert(AS.FLAT_ADDRESS <= AS.MAX_COMMON_ADDRESS &&
+         AS.GLOBAL_ADDRESS <= AS.MAX_COMMON_ADDRESS &&
+         AS.LOCAL_ADDRESS <= AS.MAX_COMMON_ADDRESS &&
+         AS.CONSTANT_ADDRESS <= AS.MAX_COMMON_ADDRESS &&
+         AS.PRIVATE_ADDRESS <= AS.MAX_COMMON_ADDRESS);
+  if (asA > AS.MAX_COMMON_ADDRESS || asB > AS.MAX_COMMON_ADDRESS)
     report_fatal_error("Pointer address space out of range");
----------------
This checking seems to belong in getAliasResult since that is where these restrictions are needed due to the implementation inside getAliasResult.


================
Comment at: lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:594
 
   return N->getAddressSpace() == AMDGPUAS::CONSTANT_BUFFER_0 + CbId;
 }
----------------
For consistency should this be AMDGPUASI.CONSTANT_BUFFER_0 ?


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:2212
 bool SITargetLowering::shouldEmitGOTReloc(const GlobalValue *GV) const {
   return (GV->getType()->getAddressSpace() == AMDGPUAS::GLOBAL_ADDRESS ||
+              GV->getType()->getAddressSpace() == AMDGPUASI.CONSTANT_ADDRESS) &&
----------------
Should this be AMDGPUASI.GLOBAL_ADDRESS for consistency?


================
Comment at: lib/Target/AMDGPU/SIISelLowering.cpp:2401
     unsigned DestAS = ASC->getDestAddressSpace();
     MFI.HasFlatLocalCasts = DestAS == AMDGPUAS::LOCAL_ADDRESS;
 
----------------
Should this be AMDGPUASI.LOCAL_ADDRESS for consistency?


================
Comment at: lib/Target/AMDGPU/SMInstructions.td:231
     static_cast<const SITargetLowering *>(getTargetLowering())->isMemOpUniform(N)) ||
     (Subtarget->getScalarizeGlobalBehavior() && Ld->getAddressSpace() == AMDGPUAS::GLOBAL_ADDRESS &&
     static_cast<const SITargetLowering *>(getTargetLowering())->isMemOpUniform(N) &&
----------------
Should this be AMDGPUASI.GLOBAL_ADDRESS for consistency?


https://reviews.llvm.org/D31284