[IPSCCP] Drop argmemonly after replacing pointer argument.

[fhahn]

Summary

This patch updates IPSCCP to drop argmemonly and
inaccessiblemem_or_argmemonly if it replaces a pointer argument.

Fixes PR46717.

[jdoerfert]

This doesn't "work" for

define internal void @ptrarg.5(i64 %arg, i32 %val) argmemonly inaccessiblemem_or_argmemonly nounwind {
  %p = inttoptr i64 %arg to i32* 
  store i32 %val, i32* %p
  ret void
}

Unsure if that is a problem in the real world but I wanted to mention it.

[fhahn]

In D84432#2169949, @jdoerfert wrote:

This doesn't "work" for

define internal void @ptrarg.5(i64 %arg, i32 %val) argmemonly inaccessiblemem_or_argmemonly nounwind {
  %p = inttoptr i64 %arg to i32* 
  store i32 %val, i32* %p
  ret void
}

Unsure if that is a problem in the real world but I wanted to mention it.

Is this valid (as in not UB)? For argmemonly, langref says ... loads and stores from objects pointed to by its pointer-typed arguments, with arbitrary offsets., but in the example we access an object not pointed to any pointer argument.

BTW, I think it would be good to have something like an attribute verifier. Catching all violations is impossible, but at least some things could be checked :)

[jdoerfert]

In D84432#2170003, @fhahn wrote:

In D84432#2169949, @jdoerfert wrote:

This doesn't "work" for

define internal void @ptrarg.5(i64 %arg, i32 %val) argmemonly inaccessiblemem_or_argmemonly nounwind {
  %p = inttoptr i64 %arg to i32* 
  store i32 %val, i32* %p
  ret void
}

Unsure if that is a problem in the real world but I wanted to mention it.

Is this valid (as in not UB)? For argmemonly, langref says ... loads and stores from objects pointed to by its pointer-typed arguments, with arbitrary offsets., but in the example we access an object not pointed to any pointer argument.

I think you're right.

BTW, I think it would be good to have something like an attribute verifier. Catching all violations is impossible, but at least some things could be checked :)

100% Agreed, patches welcome ;)

[efriedma]

llvm/lib/Transforms/Scalar/SCCP.cpp

1927

Do we also need to modify the attributes on the callsites?

[fhahn]

Also drop callsite argmemonly/inaccessiblemem_or_argmemonly attributes

llvm/lib/Transforms/Scalar/SCCP.cpp

1927

Yes, updated!

[efriedma]

LGTM

llvm/lib/Transforms/Scalar/SCCP.cpp

bool llvm::runIPSCCP(
   // constants if we have found them to be of constant values.
 
   for (Function &F : M) {
     if (F.isDeclaration())
       continue;
 
     SmallVector<BasicBlock *, 512> BlocksToErase;
 
-    if (Solver.isBlockExecutable(&F.front()))
-      for (Function::arg_iterator AI = F.arg_begin(), E = F.arg_end(); AI != E;
-           ++AI) {
-        if (!AI->use_empty() && tryToReplaceWithConstant(Solver, &*AI)) {
+    if (Solver.isBlockExecutable(&F.front())) {
+      bool ReplacedPointerArg = false;
+      for (Argument &Arg : F.args()) {
+        if (!Arg.use_empty() && tryToReplaceWithConstant(Solver, &Arg)) {
+          ReplacedPointerArg |= Arg.getType()->isPointerTy();
           ++IPNumArgsElimed;
-          continue;
-        }
-      }
+        }
+      }
+
+      // If we replaced an argument, argmemonly and
+      // inaccessiblemem_or_argmemonly do not hold any longer. Remove them.
+      if (ReplacedPointerArg) {
+        F.removeFnAttr(Attribute::ArgMemOnly);
+        F.removeFnAttr(Attribute::InaccessibleMemOrArgMemOnly);
 
     SmallPtrSet<Value *, 32> InsertedValues;
     for (BasicBlock &BB : F) {
       if (!Solver.isBlockExecutable(&BB)) {
         LLVM_DEBUG(dbgs() << "  BasicBlock Dead:" << BB);
         ++NumDeadBlocks;
 
         MadeChanges = true;

llvm/test/Transforms/SCCP/ipscp-drop-argmemonly.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --check-attributes
 ; RUN: opt -ipsccp -S %s | FileCheck %s
 
 ; Test cases to ensure argmemonly/inaccessiblemem_or_argmemonly attributes are
 ; dropped, if a function argument is replaced by a constant.
 ;
 ; PR46717
 
 @g = internal global i32 0
 
 ; Here the pointer argument %arg will be replaced by a constant. We need to
 ; drop argmemonly.
 define internal void @ptrarg.1(i32* %arg, i32 %val) argmemonly nounwind {
-; CHECK: Function Attrs: argmemonly nounwind
+; CHECK: Function Attrs: nounwind
 ; CHECK-LABEL: @ptrarg.1(
 ; CHECK-NEXT:    store i32 10, i32* @g, align 4
 ; CHECK-NEXT:    ret void
 ;
   store i32 %val, i32* %arg
   ret void
 }
 
;
   tail call void @ptrarg.2(i32* %ptr, i32 10)
   ret void
 }
 
 
 ; Here the pointer argument %arg will be replaced by a constant. We need to
 ; drop inaccessiblemem_or_argmemonly.
 define internal void @ptrarg.3(i32* %arg, i32 %val) inaccessiblemem_or_argmemonly nounwind {
-; CHECK: Function Attrs: inaccessiblemem_or_argmemonly nounwind
+; CHECK: Function Attrs: nounwind
 ; CHECK-LABEL: @ptrarg.3(
 ; CHECK-NEXT:    store i32 10, i32* @g, align 4
 ; CHECK-NEXT:    ret void
 ;
   store i32 %val, i32* %arg
   ret void
 }
 
;
   tail call void @ptrarg.4(i32* %ptr, i32 10)
   ret void
 }
 
 
 ; Here the pointer argument %arg will be replaced by a constant. We need to
 ; drop inaccessiblemem_or_argmemonly.
 define internal void @ptrarg.5(i32* %arg, i32 %val) argmemonly inaccessiblemem_or_argmemonly nounwind {
-; CHECK: Function Attrs: argmemonly inaccessiblemem_or_argmemonly nounwind
+; CHECK: Function Attrs: nounwind
 ; CHECK-LABEL: @ptrarg.5(
 ; CHECK-NEXT:    store i32 10, i32* @g, align 4
 ; CHECK-NEXT:    ret void
 ;
   store i32 %val, i32* %arg
   ret void
 }