Do not let Value::stripPointerCasts() look through returned arg functions.

[jroelofs]

Summary

We can allow this in alias analysis context, but it should be avoided for the general case where looking through would cause a semantic difference, i.e. in an objc_retain call (which will soon be marked thisreturn [1]). FWIW, the "bug" here was introduced in [2].

1: https://reviews.llvm.org/D105671
2: https://reviews.llvm.org/D9383

[nikic]

We can allow this in alias analysis context, but it should be avoided for the general case where looking through would cause a semantic difference, i.e. in an objc_retain call (which will soon be marked thisreturn [1]). FWIW, the "bug" here was introduced in [2].

returned is not supposed to have any semantic difference. In fact, we will replace calls with the returned attribute with the corresponding argument.

I think this change may still make sense because looking through returned in various analyses is pretty useless, because we can just wait for the call result to be replaced with the argument. But I don't think the current behavior should be a correctness issue.

[jroelofs]

Found a simpler way to work around the issue that motivated this in D105671.

I don't see much value in this patch now.

llvm/include/llvm/IR/Value.h

public:
   /// is called on a non-pointer value, it returns 'this'.
   const Value *stripPointerCastsSameRepresentation() const;
   Value *stripPointerCastsSameRepresentation() {
     return const_cast<Value *>(static_cast<const Value *>(this)
                                    ->stripPointerCastsSameRepresentation());
   }
 
   /// Strip off pointer casts, all-zero GEPs, single-argument phi nodes and
-  /// invariant group info.
+  /// invariant group info.  Looks through returned arg functions.
   ///
   /// Returns the original uncasted value.  If this is called on a non-pointer
   /// value, it returns 'this'. This function should be used only in
   /// Alias analysis.
   const Value *stripPointerCastsForAliasAnalysis() const;
   Value *stripPointerCastsForAliasAnalysis() {
     return const_cast<Value *>(static_cast<const Value *>(this)
                                    ->stripPointerCastsForAliasAnalysis());

llvm/lib/IR/Value.cpp

if (auto *GEP = dyn_cast<GEPOperator>(V)) {
       V = cast<Operator>(V)->getOperand(0);
     } else if (StripKind == PSK_ZeroIndicesAndAliases && isa<GlobalAlias>(V)) {
       V = cast<GlobalAlias>(V)->getAliasee();
     } else if (StripKind == PSK_ForAliasAnalysis && isa<PHINode>(V) &&
                cast<PHINode>(V)->getNumIncomingValues() == 1) {
       V = cast<PHINode>(V)->getIncomingValue(0);
     } else {
       if (const auto *Call = dyn_cast<CallBase>(V)) {
-        if (const Value *RV = Call->getReturnedArgOperand()) {
+        if (const Value *RV = Call->getReturnedArgOperand();
+            RV && StripKind == PSK_ForAliasAnalysis) {
           V = RV;
           continue;
         }
         // The result of launder.invariant.group must alias it's argument,
         // but it can't be marked with returned attribute, that's why it needs
         // special case.
         if (StripKind == PSK_ForAliasAnalysis &&
             (Call->getIntrinsicID() == Intrinsic::launder_invariant_group ||

llvm/unittests/IR/InstructionsTest.cpp

TEST(InstructionsTest, CastCAPI) {
   Type *V2Int32PtrAS1Ty = FixedVectorType::get(Int32PtrAS1Ty, 2);
   Type *V2Int32PtrTy = FixedVectorType::get(Int32PtrTy, 2);
   const Constant *CV2ptr32 = Constant::getNullValue(V2Int32PtrTy);
 
   EXPECT_EQ(LLVMAddrSpaceCast, LLVMGetCastOpcode(wrap(CV2ptr32), true,
                                                  wrap(V2Int32PtrAS1Ty), true));
 }
 
+TEST(InstructionsTest, StripPointerCasts) {
+  LLVMContext C;
+  Type *I64PtrTy = Type::getInt64PtrTy(C);
+  Type *I32PtrTy = Type::getInt32PtrTy(C);
+
+  Module M("M", C);
+  FunctionType *FTy =
+      FunctionType::get(I64PtrTy, {I64PtrTy}, false);
+  auto *F = Function::Create(FTy, Function::ExternalLinkage, "", M);
+  auto *BB = BasicBlock::Create(C, "bb", F);
+  IRBuilder<> Builder(C);
+  Builder.SetInsertPoint(BB);
+
+  Value *A0 = F->getArg(0);
+  Function *SSACopy = Intrinsic::getDeclaration(&M, Intrinsic::ssa_copy, {A0->getType()});
+  CallInst *Call = Builder.CreateCall(SSACopy, {A0});
+  Value *Cast = Builder.CreateBitCast(Call, I32PtrTy);
+
+  // When stripping for alias analysis, it is okay to look through returned
+  // argument functions, since the pointer is unchanged.
+  EXPECT_EQ(Cast->stripPointerCastsForAliasAnalysis(), A0);
+
+  // Otherwise, this is not okay.
+  EXPECT_EQ(Cast->stripPointerCasts(), Call);
+  EXPECT_EQ(Cast->stripPointerCastsAndAliases(), Call);
+  EXPECT_EQ(Cast->stripPointerCastsSameRepresentation(), Call);
+  EXPECT_EQ(Cast->stripInBoundsConstantOffsets(), Call);
+}
+
 TEST(InstructionsTest, VectorGep) {
   LLVMContext C;
 
   // Type Definitions
   Type *I8Ty = IntegerType::get(C, 8);
   Type *I32Ty = IntegerType::get(C, 32);
   PointerType *Ptri8Ty = PointerType::get(I8Ty, 0);
   PointerType *Ptri32Ty = PointerType::get(I32Ty, 0);