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

[bjope]

Summary

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

[bjope]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp

156

I'm not 100% sure about this. https://llvm.org/docs/LangRef.html#global-variables says that "code could assume that the globals are densely packed in their section and try to iterate over them as an array", so does that mean that for example a global variable @foo can be accessed using a global variable @bar as base ptr and with (@bar - @foo) as offset?

[nikic]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp

156

Accessing an object using a pointer that is based on a different object is undefined behavior under our pointer aliasing rules.

[bjope]

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp

156

@nikic: Thanks! Well, then my assumption here seem to hold (at least for LLVM IR).

Although, afaict LangRef covers LLVM IR, and not neccessarily MIR. And SelectionDAG/DAGCombiner is a bit in-between. So when LangRef talks about pointer aliasing rules for constants, then I figure that there is no guarantee that those rules also holds for ConstantPoolSDNode objects, etc.

[bjope]

ping

[niravd]

LGTM.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp

bool BaseIndexOffset::computeAliasing(const SDNode *Op0,
   bool IsCV1 = isa<ConstantPoolSDNode>(BasePtr1.getBase());
 
   if ((IsFI0 || IsGV0 || IsCV0) && (IsFI1 || IsGV1 || IsCV1)) {
     // We can derive NoAlias In case of mismatched base types.
     if (IsFI0 != IsFI1 || IsGV0 != IsGV1 || IsCV0 != IsCV1) {
       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()) {
+    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.
 }
 
 bool BaseIndexOffset::contains(const SelectionDAG &DAG, int64_t BitSize,
                                const BaseIndexOffset &Other,
                                int64_t OtherBitSize, int64_t &BitOffset) const {
   int64_t Offset;
   if (!equalBaseIndex(Other, DAG, Offset))

llvm/unittests/CodeGen/SelectionDAGAddressAnalysisTest.cpp

TEST_F(SelectionDAGAddressAnalysisTest, sameFrameObject) {
   bool IsAlias;
   bool IsValid = BaseIndexOffset::computeAliasing(
       Store.getNode(), NumBytes, Store.getNode(), NumBytes, *DAG, IsAlias);
 
   EXPECT_TRUE(IsValid);
   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);
   // <4 x i8>
   auto VecVT = EVT::getVectorVT(Context, Int8VT, 4);
   // <2 x i8>
   auto SubVecVT = EVT::getVectorVT(Context, Int8VT, 2);
   SDValue FIPtr = DAG->CreateStackTemporary(VecVT);