[PATCH] D31284: [AMDGPU] Get address space mapping by target triple
Yaxun Liu via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 24 19:05:50 PDT 2017
yaxunl 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.
I will fix those inconsistencies.
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).
You are right. I will use two static arrays and choose the right one in constructor.
We need to include Region memory for the index to be continuous from 0 to 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.
will move it there.
More information about the llvm-commits