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

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added reviewers: jdoerfert, asbirlea.
bjope added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:156
+    if (IsGV0 && IsGV1 &&
+        (isa<GlobalAlias>(cast<GlobalAddressSDNode>(
+                              BasePtr0.getBase())->getGlobal()) ||
----------------
Maybe it is safer to look for GlobalIndirectSymbol here instead of GlobalAlias (as that would include any other indirect reference)?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:168
+    // dereferenced)?
+    if (BasePtr0.getIndex() == BasePtr1.getIndex()) {
+      IsAlias = false;
----------------
bjope wrote:
> 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).
I think that it maybe would be safer (and more correct) to check if we access two different GlobalValue here, and then derive NoAlias. Wouldn't it be weird to access one global using the address of another GlobalValue (the actual layout in memory between globals isn't given, or is it)?

(Although my intention with this patch has been to avoid the miscompiles for GlobalAlias:es, and I had planned to leave this mess with deriving NoAlias based on having identival Index in the BaseOffsetIndex for the future.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110064