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

[Yaxun Liu via Phabricator via llvm-commits]

yaxunl marked 5 inline comments as done.
yaxunl added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp:62-63
+  };
+  ASAliasRules = AS.FLAT_ADDRESS == 0 ? &ASAliasRulesGenIsZero :
+      &ASAliasRulesPrivIsZero;
+}
----------------
t-tye wrote:
> Would it be better to simply assert the assumption built into the tables that the address space indexes are exactly as expected, and that MAX_COMMON_ADDRESS is less than or equal to the table size?
> 
> ```
> assert(AS.MAX_COMMON_ADDRESS <= 5);
> if (AS.FLAT_ADDRESS == 0) {
>   assert(AS.FLAT_ADDRESS == 0 &&
>            AS.GLOBAL_ADDRESS == 1 &&
>            AS.REGION_ADDRESS == 2 &&
>            AS.LOCAL_ADDRESS == 3 &&
>            AS.CONSTANT_ADDRESS == 4 &&
>            AS.PRIVATE_ADDRESS == 5);
>   ASAliasRules = &ASAliasRulesGenIsZero;
> } else {
>   assert(AS.PRIVATE_ADDRESS == 0 &&
>            AS.GLOBAL_ADDRESS == 1 &&
>            AS.CONSTANT_ADDRESS == 2 &&
>            AS.LOCAL_ADDRESS == 3 &&
>            AS.FLAT_ADDRESS == 4 &&
>            AS.REGION_ADDRESS == 5);
>   ASAliasRules = &ASAliasRulesPrivIsZero;
> }
> ```
yes. will do.


================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.h:53
+    ASAliasRulesTy(AMDGPUAS AS_);
+    ASAliasRulesTy(ASAliasRulesTy &R):AS(R.AS), ASAliasRules(R.ASAliasRules){}
+    AliasResult getAliasResult(unsigned AS1, unsigned AS2);
----------------
t-tye wrote:
> Should this be ASAliasRulesTy(const ASAliasRulesTy &R)? Does if even need to be defined since it is exactly the same as the default copy constructor?
You are right. We don't need this. Will remove it.


================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.h:54
+    ASAliasRulesTy(ASAliasRulesTy &R):AS(R.AS), ASAliasRules(R.ASAliasRules){}
+    AliasResult getAliasResult(unsigned AS1, unsigned AS2);
+  private:
----------------
t-tye wrote:
> Should this be a const member since it does not change ASAliasRulesTy?
Will make it const.


https://reviews.llvm.org/D31284