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

[Tony Tye via Phabricator via llvm-commits]

t-tye accepted this revision.
t-tye added a comment.
This revision is now accepted and ready to land.

Minor suggestions.

Presumably the concern to avoid linking LLVM IR with different settings of the address space mapping is enforced because each has a different data layout.

Otherwise:

LGTM



================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp:60
+  /* Constant */ {MayAlias, NoAlias , NoAlias , NoAlias , MayAlias, NoAlias},
+  /* Privat   */ {MayAlias, NoAlias , NoAlias , NoAlias , NoAlias , MayAlias}
+  };
----------------
Privat->Private


================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp:62-63
+  };
+  ASAliasRules = AS.FLAT_ADDRESS == 0 ? &ASAliasRulesGenIsZero :
+      &ASAliasRulesPrivIsZero;
+}
----------------
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;
}
```


================
Comment at: lib/Target/AMDGPU/AMDGPUAliasAnalysis.cpp:68-73
+  assert(AS.FLAT_ADDRESS <= AS.MAX_COMMON_ADDRESS &&
+         AS.GLOBAL_ADDRESS <= AS.MAX_COMMON_ADDRESS &&
+         AS.REGION_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);
----------------
Move to do once in the constructor.


================
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);
----------------
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?


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


https://reviews.llvm.org/D31284