[PATCH] D124787: [NVPTX] Implement NVPTX AliasAnalysis

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Looks fine from a cursory look. To double check, this property also holds up in the presence of addrspacecasts? Note that AA will skip past those, so if you compare `ptr addrspace(1) %x` and `addrspacecast (ptr addrspace(5) %y to ptr addrspace(1))`, this will perform a comparison of `ptr addrspace(1) %x` and `ptr addrspace(5) %y`. I assume that the addrspacecast would just be considered illegal in the relevant cases?



================
Comment at: llvm/lib/Target/NVPTX/NVPTXAliasAnalysis.cpp:83
+
+  if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(Base))
+    if (GV->isConstant())
----------------
Shouldn't be necessary, as the BasicAA implementation already covers this.


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAliasAnalysis.cpp:64
+  if (Result == AliasResult::NoAlias)
+    return Result;
+
----------------
asavonic wrote:
> tra wrote:
> > Nit. I'd rephrase it a bit and put `alias()` call closer to the condition check. 
> > ```
> >   if (getAliasResult(AS1, AS2) != AliasResult::NoAlias) 
> >       return AAResultBase::alias(Loc1, Loc2, AAQI);
> >   return  AliasResult::NoAlias;
> > ```
> > Maybe, even fold it into a single `return (...== NoAlias) ? NoAlias : alias();`
> Done.
Can't you just `return getAliasResult(AS1, AS2)` here? Calling `AAResultBase` methods is not necessary, it'll always return MayAlias (there are some confusing comments about this in other AA implemementations, but chaining of AA providers happens at a different level nowadays).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124787/new/

https://reviews.llvm.org/D124787