[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;
+        }
+      }
+
+      // If we replaced an argument, the argmemonly and
+      // inaccessiblemem_or_argmemonly attributes do not hold any longer. Remove
+      // them from both the function and callsites.
+      if (ReplacedPointerArg) {
+        SmallVector<Attribute::AttrKind, 2> AttributesToRemove = {
+            Attribute::ArgMemOnly, Attribute::InaccessibleMemOrArgMemOnly};
+        for (auto Attr : AttributesToRemove)
+          F.removeFnAttr(Attr);
+
+        for (User *U : F.users()) {
+          auto *CB = dyn_cast<CallBase>(U);
+          if (!CB || CB->getCalledFunction() != &F)
             continue;
-        }
-      }
+
+          for (auto Attr : AttributesToRemove)
+            CB->removeAttribute(AttributeList::FunctionIndex, Attr);
+        }
 
     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
 }
 

 ;
   store i32 %val, i32* %arg
   ret void
 }
 
 define i32 @caller.6.cs.attributes(i32 %n) {
 ; CHECK-LABEL: @caller.6.cs.attributes(
 ; CHECK-NEXT:    store i32 1, i32* @g, align 4
-; CHECK-NEXT:    tail call void @ptrarg.5(i32* @g, i32 10) [[ARGMEMONLY_INACCESSIBLEMEM_OR_ARGMEMONLY_NOUNWIND:#[0-9]+]]
-; CHECK-NEXT:    tail call void @ptrarg.5(i32* @g, i32 10) [[INACCESSIBLEMEM_OR_ARGMEMONLY_NOUNWIND:#[0-9]+]]
-; CHECK-NEXT:    tail call void @ptrarg.5(i32* @g, i32 10) [[ARGMEMONLY_NOUNWIND:#[0-9]+]]
+; CHECK-NEXT:    tail call void @ptrarg.5(i32* @g, i32 10) [[NOUNWIND:#[0-9]+]]
+; CHECK-NEXT:    tail call void @ptrarg.5(i32* @g, i32 10) [[NOUNWIND:#[0-9]+]]
+; CHECK-NEXT:    tail call void @ptrarg.5(i32* @g, i32 10) [[NOUNWIND:#[0-9]+]]
 ; CHECK-NEXT:    tail call void @ptrarg.5(i32* @g, i32 10) [[NOUNWIND:#[0-9]+]]
 ; CHECK-NEXT:    [[G_VAL:%.*]] = load i32, i32* @g, align 4
 ; CHECK-NEXT:    ret i32 [[G_VAL]]
 ;
   store i32 1, i32* @g
   tail call void @ptrarg.5(i32* @g, i32 10) argmemonly inaccessiblemem_or_argmemonly nounwind
   tail call void @ptrarg.5(i32* @g, i32 10) inaccessiblemem_or_argmemonly nounwind
   tail call void @ptrarg.5(i32* @g, i32 10) argmemonly nounwind
   tail call void @ptrarg.5(i32* @g, i32 10) nounwind
   %g.val = load i32, i32* @g
   ret i32 %g.val
 }
 
-; CHECK-DAG: [[ARGMEMONLY_INACCESSIBLEMEM_OR_ARGMEMONLY_NOUNWIND]] = { argmemonly inaccessiblemem_or_argmemonly nounwind }
-; CHECK-DAG: [[INACCESSIBLEMEM_OR_ARGMEMONLY_NOUNWIND]] = { inaccessiblemem_or_argmemonly nounwind }
-; CHECK-DAG: [[ARGMEMONLY_NOUNWIND]] = { argmemonly nounwind }
-; CHECK-DAG: [[NOUNWIND]] = { nounwind }
+; CHECK: [[NOUNWIND]] = { nounwind }