[NewGVN] Remove returned arg simplification

[ManuelJBrito]

Replacing uses of the return value with the argument is already handled in other passes, additionally it causes issues with memory value numbering when the call is a memory defining intrinsic.
fixes #159918

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (ManuelJBrito)

Changes

In NewGVN we value number memory versions, therefore simplifying a memory defining call to the returned argument is nonsensical.
fixes #159918

---

Full diff: https://github.com/llvm/llvm-project/pull/161865.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+3-1)
• (added) llvm/test/Transforms/NewGVN/pr159918.ll (+21)

diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 9d4fb79416596..561c5341e57de 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -1647,7 +1647,9 @@ NewGVN::performSymbolicPredicateInfoEvaluation(BitCastInst *I) const {
 NewGVN::ExprResult NewGVN::performSymbolicCallEvaluation(Instruction *I) const {
   auto *CI = cast<CallInst>(I);
   if (auto *II = dyn_cast<IntrinsicInst>(I)) {
-    if (auto *ReturnedValue = II->getReturnedArgOperand())
+    auto *ReturnedValue = II->getReturnedArgOperand();
+    auto *MemDef = dyn_cast_or_null<MemoryDef>(getMemoryAccess(I));
+    if (ReturnedValue && !MemDef)
       return ExprResult::some(createVariableOrConstant(ReturnedValue));
   }
 
diff --git a/llvm/test/Transforms/NewGVN/pr159918.ll b/llvm/test/Transforms/NewGVN/pr159918.ll
new file mode 100644
index 0000000000000..3fad6e66d11ac
--- /dev/null
+++ b/llvm/test/Transforms/NewGVN/pr159918.ll
@@ -0,0 +1,21 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 6
+; RUN: opt -S -passes=newgvn < %s | FileCheck %s
+
+; Don't use returned argument in memory defining intrinsics.
+define void @wombat(ptr %arg) {
+; CHECK-LABEL: define void @wombat(
+; CHECK-SAME: ptr [[ARG:%.*]]) {
+; CHECK-NEXT:    [[LOAD:%.*]] = load ptr, ptr [[ARG]], align 8
+; CHECK-NEXT:    [[CALL:%.*]] = call ptr @llvm.objc.retain(ptr [[LOAD]])
+; CHECK-NEXT:    store ptr [[CALL]], ptr [[ARG]], align 8
+; CHECK-NEXT:    ret void
+;
+  %load = load ptr, ptr %arg, align 8
+  %call = call ptr @llvm.objc.retain(ptr %load)
+  store ptr %call, ptr %arg, align 8
+  ret void
+}
+
+declare ptr @llvm.objc.retain(ptr returned) #0
+
+attributes #0 = { nounwind }

[nikic]

Hm, can we just delete this entirely?

We generally replace uses of the return value with the argument as a separate fold already, so I don't think there's really a need for NewGVN to handle this.

[ManuelJBrito]

Yes, I guess this was just a remnant of the old PredicateInfo with ssa.copy.
Does it make sense to keep the test?

[nikic]

Yes, I guess this was just a remnant of the old PredicateInfo with ssa.copy. Does it make sense to keep the test?

Yeah, extra test coverage isn't going to hurt...

[nikic]

LGTM, but PR description needs an update.