[PATCH] D124787: [NVPTX] Implement NVPTX AliasAnalysis

[Andrew Savonichev via Phabricator via llvm-commits]

asavonic added a comment.

This took a while, I'm sorry for not following up earlier.

In D124787#3614613 <https://reviews.llvm.org/D124787#3614613>, @nikic wrote:

> 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?

Yes, pointers in different address spaces do not alias unless at least one of them is in generic (flat) address space. Address space casts can only cast from/to generic address space, otherwise they are invalid.

> Note that AA will skip past those

This might be an opportunity to improve the analysis. When we skip addrspacecasts from two pointers, we may end up with pointers in generic address space, and the result is always `MayAlias`. We can get better results from original pointers if they are in different address spaces (e.g. local and global AS never alias).

In D124787#3613322 <https://reviews.llvm.org/D124787#3613322>, @tschuett wrote:

> Could that be a more general solution? You ask the target if address spaces A and B could alias?

We can add a simple check to TTI, and integrate it to BasicAA, but I think the current interface is simple enough already. Having access to MemoryLocation can be important: we can traverse a chain of address space casts and make a better AA decision (see AMDGPU AA for example). Also, `pointsToConstantMemory` check is done via AA anyway, so we have to implement a target-specific AA to customize it.



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


================
Comment at: llvm/lib/Target/NVPTX/NVPTXAliasAnalysis.cpp:64
+  if (Result == AliasResult::NoAlias)
+    return Result;
+
----------------
nikic wrote:
> 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).
Right, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124787