[PATCH] D110256: [SelectionDAG] Replace error prone index check in BaseIndexOffset::computeAliasing

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope created this revision.
bjope added reviewers: courbet, niravd, efriedma, jdoerfert, asbirlea.
Herald added subscribers: jeroen.dobbelaere, ecnelises, arphaman, hiraditya.
bjope requested review of this revision.
Herald added a project: LLVM.

Deriving NoAlias based on having the same index in two BaseIndexOffset
expressions seemed weird (and as shown in the added unittest the
correctness of doing so depended on undocumented pre-conditions that
the user of BaseIndexOffset::computeAliasing would need to take care
of.

This patch removes the code that dereived NoAlias based on indices
being the same. As a compensation, to avoid regressions/diffs in
various lit test, we also add a new check. The new check derives
NoAlias in case the two base pointers are based on two different
GlobalValue:s (neither of them being a GlobalAlias).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D110256

Files:
  llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
  llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp


Index: llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp
===================================================================
--- llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp
+++ llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp
@@ -121,6 +121,31 @@
   EXPECT_TRUE(IsAlias);
 }
 
+TEST_F(SelectionDAGAddressAnalysisTest, sameFrameObjectUnknownSize) {
+  SDLoc Loc;
+  auto Int8VT = EVT::getIntegerVT(Context, 8);
+  auto VecVT = EVT::getVectorVT(Context, Int8VT, 4);
+  SDValue FIPtr = DAG->CreateStackTemporary(VecVT);
+  int FI = cast<FrameIndexSDNode>(FIPtr.getNode())->getIndex();
+  MachinePointerInfo PtrInfo = MachinePointerInfo::getFixedStack(*MF, FI);
+  TypeSize Offset = TypeSize::Fixed(0);
+  SDValue Value = DAG->getConstant(0, Loc, VecVT);
+  SDValue Index = DAG->getMemBasePlusOffset(FIPtr, Offset, Loc);
+  SDValue Store = DAG->getStore(DAG->getEntryNode(), Loc, Value, Index,
+                                PtrInfo.getWithOffset(Offset));
+
+  // Maybe unlikely that BaseIndexOffset::computeAliasing is used with the
+  // optional NumBytes being unset like in this test, but it would be confusing
+  // if that function determined IsAlias=false here.
+  Optional<int64_t> NumBytes;
+
+  bool IsAlias;
+  bool IsValid = BaseIndexOffset::computeAliasing(
+      Store.getNode(), NumBytes, Store.getNode(), NumBytes, *DAG, IsAlias);
+
+  EXPECT_FALSE(IsValid);
+}
+
 TEST_F(SelectionDAGAddressAnalysisTest, noAliasingFrameObjects) {
   SDLoc Loc;
   auto Int8VT = EVT::getIntegerVT(Context, 8);
Index: llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
+++ llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
@@ -150,24 +150,20 @@
       IsAlias = false;
       return true;
     }
-    // We cannot safely determine whether the pointers alias if we compare two
-    // global values and at least one is a GlobalAlias.
-    if (IsGV0 && IsGV1 &&
-        (isa<GlobalAlias>(cast<GlobalAddressSDNode>(
-                              BasePtr0.getBase())->getGlobal()) ||
-         isa<GlobalAlias>(cast<GlobalAddressSDNode>(
-                              BasePtr1.getBase())->getGlobal())))
-        return false;
-    // If checkable indices we can check they do not alias.
-    // FIXME: Please describe this a bit better. Looks weird to say that there
-    // is no alias if the indices are the same. Is this code assuming that
-    // someone has checked that the base isn't the same as a precondition? And
-    // what about offsets? And what about NumBytes0 and NumBytest1 (can we
-    // really derive NoAlias here if we do not even know how many bytes that are
-    // dereferenced)?
-    if (BasePtr0.getIndex() == BasePtr1.getIndex()) {
-      IsAlias = false;
-      return true;
+    if (IsGV0 && IsGV1) {
+      auto *GV0 = cast<GlobalAddressSDNode>(BasePtr0.getBase())->getGlobal();
+      auto *GV1 = cast<GlobalAddressSDNode>(BasePtr1.getBase())->getGlobal();
+      // It doesn't make sense to access one global value using another globals
+      // values address, so we can assume that there is no aliasing in case of
+      // two different globals (unless we have symbols that may indirectly point
+      // to each other).
+      // FIXME: This is perhaps a bit too defensive. We could try to follow the
+      // chain with aliasee information for GlobalAlias variables to find out if
+      // we indirect symbols may alias or not.
+      if (GV0 != GV1 && !isa<GlobalAlias>(GV0) && !isa<GlobalAlias>(GV1)) {
+        IsAlias = false;
+        return true;
+      }
     }
   }
   return false; // Cannot determine whether the pointers alias.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D110256.374234.patch
Type: text/x-patch
Size: 3768 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210922/7dc97e07/attachment.bin>