From 8ed0e6b2cf941aa29628acdf718e82618d60bfc0 Mon Sep 17 00:00:00 2001
From: Bjorn Pettersson <bjorn.a.pettersson@ericsson.com>
Date: Mon, 20 Sep 2021 11:32:01 +0200
Subject: [PATCH] [SelectionDAG] Replace error prone index check in
 BaseIndexOffset::computeAliasing

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).

Reviewed By: niravd

Differential Revision: https://reviews.llvm.org/D110256
---
 .../SelectionDAG/SelectionDAGAddressAnalysis.cpp   | 32 ++++++++++------------
 .../CodeGen/SelectionDAGAddressAnalysisTest.cpp    | 25 +++++++++++++++++
 2 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
index 2f9a93c..6d82520 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
@@ -150,24 +150,20 @@ bool BaseIndexOffset::computeAliasing(const SDNode *Op0,
       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.
diff --git a/llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp b/llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp
index 28485a3..1a310b8ec 100644
--- a/llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp
+++ b/llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp
@@ -121,6 +121,31 @@ TEST_F(SelectionDAGAddressAnalysisTest, sameFrameObject) {
   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);
-- 
2.7.4