[PATCH] D110064: [SelectionDAG] Assume that a GlobalAlias may alias other global values

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:155
+    // global values and at least one is a GlobalAlias.
+    if (IsGV0 && IsGV1 &&
+        (isa<GlobalAlias>(cast<GlobalAddressSDNode>(
----------------
niravd wrote:
> It looks like we can walk the GlobalAlias to the referenced object. If so, we should fully walk to a non-alias global instead.
> 
As I wrote in the desciption "In the future we might improve this with a deeper analysis to look at the aliasee for the GlobalAlias etc. But that is a bit more complicated considering that we could have 'local_unnamed_addr' and situations with several 'alias' variables.".

I'd actually prefer doing a bugfix that I'm confortable with. And I didn't quite understand the part about "Aliases that are not unnamed_addr are guaranteed to have the same address as the aliasee expression. unnamed_addr ones are only guaranteed to point to the same content." from https://llvm.org/docs/LangRef.html#aliases .

But if my approach was too naive and defensive, then I wonder why we have references to GlobalAlias:es in the IR in the first place. Couldn't we just reference the aliasee instead (leaving the GlobalAlias declaration as an unused declaration)?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:168
+    // dereferenced)?
+    if (BasePtr0.getIndex() == BasePtr1.getIndex()) {
+      IsAlias = false;
----------------
niravd wrote:
> We should handle this case earlier so we can correct determine that identical global aliases do in fact alias.
Well, this if-statement derives `IsAlias=false`. So I'm just as confused about this check as your suggestion here. I needed to pull this check out from the orignal if-statment just to make it possible to add the check for global aliases before this. Otherwise it would derive that there is no alias between global aliases.

On the other hand, if this check of indices is considered as a bug in itself, to be removed, then my new check for global aliases isn't really needed either.

Removing this if-statement makes about 10 lit-tests to fail. Probably just due to getting slightly different codegen. Haven't really been able to prove that something is wrong with those tests.
Although, when adding a unittest that calls BaseIndexOffset::computeAliasing with the Op0 and Op1 being identical, but with NumBytes0 or NumBytes1 being unset (avoiding to enter the if at line 99) then computeAliasing would answer that there is no aliasing. That really seems wrong (but maybe that never happens in reality).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110064