[llvm] 8ed0e6b - [SelectionDAG] Replace error prone index check in BaseIndexOffset::computeAliasing

[Bjorn Pettersson via llvm-commits]

Author: Bjorn Pettersson
Date: 2021-10-05T12:15:55+02:00
New Revision: 8ed0e6b2cf941aa29628acdf718e82618d60bfc0

URL: https://github.com/llvm/llvm-project/commit/8ed0e6b2cf941aa29628acdf718e82618d60bfc0
DIFF: https://github.com/llvm/llvm-project/commit/8ed0e6b2cf941aa29628acdf718e82618d60bfc0.diff

LOG: [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

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp
index 2f9a93c56d6b..6d8252046501 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 
diff erent 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 28485a34d9e4..1a310b8ec9c2 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);