[ValueTracking] Accept vectors of pointer in GetUnderlyingObject utility

[skatkov]

Summary

GEP and BitCast instructions may produce a pointer of vectors from
underlying object. So it is valid for GetUnderlyingObject utility to
accept vector of pointers and detect underlying object which might be
instruction of vector or non-vector type.

[arsenm]

Needs tests. I'm also not sure making this work for vectors necessarily makes sense

[skatkov]

Thank you for your comment. Unfortunately till now I failed to create a test showing the benefit from this change in a pass (I tried). I can make one more try.
The problem I'm trying to solve is the following: in downstream pass I have a BitCast instruction which is actually originated from allocation but has a vector type gotten using gep + bitcast from allocation.
I'd like to know the fact that it is an underlying allocation but now utility function just ignore instructions with vector of pointers.

[arsenm]

There are ValueTracking unittests already you could fall back on putting a test there, but I think a pass using this would be better

[skatkov]

Thank you! I've added two unit tests.

[skatkov]

ping. Can I do anything to proceed with this change?

[arsenm]

unittests/Analysis/ValueTrackingTest.cpp

318–322

It looks like a bug this IR is accepted by the verifier at all. This is a bitcast between different sized objects

[arsenm]

unittests/Analysis/ValueTrackingTest.cpp

318–322

I think fixing this is a prerequisite for making any changes like this to avoid issues

[skatkov]

unittests/Analysis/ValueTrackingTest.cpp

318–322

what if the size of the pointer is 32 bit?

[skatkov]

unittests/Analysis/ValueTrackingTest.cpp

318–322

It seems I'm not right here...

[arsenm]

unittests/Analysis/ValueTrackingTest.cpp

318–322

This is a cast between a 64-bit pointer and 2 64-bit pointers

[skatkov]

Test is updated to be valid.

[skatkov]

Verifier fix landed, can we proceed with this change?

[arsenm]

unittests/Analysis/ValueTrackingTest.cpp

318–322

This still looks invalid to me. We don't allow bitcast between pointers and FP types for example.

[jdoerfert]

What is supposed to happen if the elements in the pointer vector have different underlying objects?
We need to document that in the comment and test it.

unittests/Analysis/ValueTrackingTest.cpp

344

Isn't this index out-of-bounds. If so, that is a problem.

[skatkov]

Sorry guys, I'm not working on this at the moment. I can abandon this review for a while if it mislead anyone.

lib/Analysis/ValueTracking.cpp

static bool isSameUnderlyingObjectInLoop(const PHINode *PN,
   if (auto *Load = dyn_cast<LoadInst>(PrevValue))
     if (!L->isLoopInvariant(Load->getPointerOperand()))
       return false;
   return true;
 }
 
 Value *llvm::GetUnderlyingObject(Value *V, const DataLayout &DL,
                                  unsigned MaxLookup) {
-  if (!V->getType()->isPointerTy())
+  if (!V->getType()->isPtrOrPtrVectorTy())
     return V;
   for (unsigned Count = 0; MaxLookup == 0 || Count < MaxLookup; ++Count) {
     if (GEPOperator *GEP = dyn_cast<GEPOperator>(V)) {
       V = GEP->getPointerOperand();
     } else if (Operator::getOpcode(V) == Instruction::BitCast ||
                Operator::getOpcode(V) == Instruction::AddrSpaceCast) {
       V = cast<Operator>(V)->getOperand(0);
     } else if (GlobalAlias *GA = dyn_cast<GlobalAlias>(V)) {
if (GEPOperator *GEP = dyn_cast<GEPOperator>(V)) {
         // TODO: Acquire a DominatorTree and AssumptionCache and use them.
         if (Value *Simplified = SimplifyInstruction(I, {DL, I})) {
           V = Simplified;
           continue;
         }
 
       return V;
     }
-    assert(V->getType()->isPointerTy() && "Unexpected operand type!");
+    assert(V->getType()->isPtrOrPtrVectorTy() && "Unexpected operand type!");
   }
   return V;
 }
 
 void llvm::GetUnderlyingObjects(Value *V, SmallVectorImpl<Value *> &Objects,
                                 const DataLayout &DL, LoopInfo *LI,
                                 unsigned MaxLookup) {
   SmallPtrSet<Value *, 4> Visited;

unittests/Analysis/ValueTrackingTest.cpp

TEST(ValueTracking, ComputeKnownMulBits) {
 
   auto *RVal =
       cast<ReturnInst>(F->getEntryBlock().getTerminator())->getOperand(0);
   auto Known = computeKnownBits(RVal, M->getDataLayout());
   ASSERT_FALSE(Known.hasConflict());
   EXPECT_EQ(Known.One.getZExtValue(), 32u);
   EXPECT_EQ(Known.Zero.getZExtValue(), 95u);
 }
+
+TEST(ValueTracking, GetUnderlyingObjectForBitcastVector) {
+  StringRef Assembly = "define <1 x i32*> @f() { "
+                       "  %a = alloca i64 "
+                       "  %b = bitcast i64* %a to <1 x i32*>"
+                       "  ret <1 x i32*> %b "
+                       "} ";
+
+  LLVMContext Context;
+  SMDiagnostic Error;
+  auto M = parseAssemblyString(Assembly, Error, Context);
+  assert(M && "Bad assembly?");
+
+  const DataLayout &DL = M->getDataLayout();
+
+  auto *F = M->getFunction("f");
+  assert(F && "Bad assembly?");
+
+  auto &BB = F->getEntryBlock();
+
+  auto *RVal = cast<ReturnInst>(BB.getTerminator())->getOperand(0);
+  auto *Alloc = GetUnderlyingObject(RVal, DL);
+  EXPECT_EQ(Alloc, &*BB.begin());
+}
+
+TEST(ValueTracking, GetUnderlyingObjectForGepVector) {
+  StringRef Assembly = "define <2 x i64*> @f() { "
+                       "  %a = alloca i64 "
+                       "  %g = getelementptr inbounds i64, i64* %a, <2 x i64> <i64 0, i64 4> "
+                       "  ret <2 x i64*> %g"
+                       "} ";
+
+  LLVMContext Context;
+  SMDiagnostic Error;
+  auto M = parseAssemblyString(Assembly, Error, Context);
+  assert(M && "Bad assembly?");
+
+  const DataLayout &DL = M->getDataLayout();
+
+  auto *F = M->getFunction("f");
+  assert(F && "Bad assembly?");
+
+  auto &BB = F->getEntryBlock();
+
+  auto *RVal = cast<ReturnInst>(BB.getTerminator())->getOperand(0);
+  auto *Alloc = GetUnderlyingObject(RVal, DL);
+  EXPECT_EQ(Alloc, &*BB.begin());
+}