[clang] Add missing readonly/readnone annotations

[qchateau]

When arg memory effects are lost due to indirect arguments, apply readonly/readnone attribute to the other pointer arguments of the function.

This should address #157693

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-codegen

Author: Quentin Chateau (qchateau)

Changes

When arg memory effects are lost due to indirect arguments, apply readonly/readnone attribute to the other pointer arguments of the function.

This should address #157693

---

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

2 Files Affected:

• (modified) clang/lib/CodeGen/CGCall.cpp (+34)
• (modified) clang/test/CodeGen/struct-passing.c (+7-1)

diff --git a/clang/lib/CodeGen/CGCall.cpp b/clang/lib/CodeGen/CGCall.cpp
index 0b2fce4244fb6..866894fb80946 100644
--- a/clang/lib/CodeGen/CGCall.cpp
+++ b/clang/lib/CodeGen/CGCall.cpp
@@ -2438,6 +2438,7 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
 
   // Some ABIs may result in additional accesses to arguments that may
   // otherwise not be present.
+  std::optional<llvm::Attribute::AttrKind> MemAttrForPtrArgs;
   auto AddPotentialArgAccess = [&]() {
     llvm::Attribute A = FuncAttrs.getAttribute(llvm::Attribute::Memory);
     if (A.isValid())
@@ -2499,11 +2500,13 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
       // gcc specifies that 'const' functions have greater restrictions than
       // 'pure' functions, so they also cannot have infinite loops.
       FuncAttrs.addAttribute(llvm::Attribute::WillReturn);
+      MemAttrForPtrArgs = llvm::Attribute::ReadNone;
     } else if (TargetDecl->hasAttr<PureAttr>()) {
       FuncAttrs.addMemoryAttr(llvm::MemoryEffects::readOnly());
       FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
       // gcc specifies that 'pure' functions cannot have infinite loops.
       FuncAttrs.addAttribute(llvm::Attribute::WillReturn);
+      MemAttrForPtrArgs = llvm::Attribute::ReadOnly;
     } else if (TargetDecl->hasAttr<NoAliasAttr>()) {
       FuncAttrs.addMemoryAttr(llvm::MemoryEffects::inaccessibleOrArgMemOnly());
       FuncAttrs.addAttribute(llvm::Attribute::NoUnwind);
@@ -3011,6 +3014,37 @@ void CodeGenModule::ConstructAttributeList(StringRef Name,
   }
   assert(ArgNo == FI.arg_size());
 
+  ArgNo = 0;
+  if (MemAttrForPtrArgs) {
+    llvm::SmallSet<unsigned, 8> ArgsToSkip;
+    if (IRFunctionArgs.hasSRetArg()) {
+      ArgsToSkip.insert(IRFunctionArgs.getSRetArgNo());
+    }
+    if (IRFunctionArgs.hasInallocaArg()) {
+      ArgsToSkip.insert(IRFunctionArgs.getInallocaArgNo());
+    }
+
+    for (CGFunctionInfo::const_arg_iterator I = FI.arg_begin(),
+                                            E = FI.arg_end();
+         I != E; ++I, ++ArgNo) {
+      if (I->info.getKind() == ABIArgInfo::Indirect) {
+        unsigned FirstIRArg, NumIRArgs;
+        std::tie(FirstIRArg, NumIRArgs) = IRFunctionArgs.getIRArgs(ArgNo);
+        for (unsigned i = FirstIRArg; i < FirstIRArg + NumIRArgs; ++i) {
+          ArgsToSkip.insert(i);
+        }
+      }
+    }
+
+    llvm::FunctionType *FctType = getTypes().GetFunctionType(FI);
+    for (unsigned i = 0; i < IRFunctionArgs.totalIRArgs(); i++) {
+      if (FctType->getParamType(i)->isPointerTy() && !ArgsToSkip.contains(i)) {
+        ArgAttrs[i] =
+            ArgAttrs[i].addAttribute(getLLVMContext(), *MemAttrForPtrArgs);
+      }
+    }
+  }
+
   AttrList = llvm::AttributeList::get(
       getLLVMContext(), llvm::AttributeSet::get(getLLVMContext(), FuncAttrs),
       llvm::AttributeSet::get(getLLVMContext(), RetAttrs), ArgAttrs);
diff --git a/clang/test/CodeGen/struct-passing.c b/clang/test/CodeGen/struct-passing.c
index c8cfeb9c8168a..1934669ce7c0a 100644
--- a/clang/test/CodeGen/struct-passing.c
+++ b/clang/test/CodeGen/struct-passing.c
@@ -14,7 +14,11 @@ T1 __attribute__((pure)) f3(void);
 void __attribute__((const)) f4(T1 a);
 void __attribute__((pure)) f5(T1 a);
 
-void *ps[] = { f0, f1, f2, f3, f4, f5 };
+// NOTE: The int parameters verifies non-ptr parameters are not a problem
+T1 __attribute__((const)) f6(void*, int);
+T1 __attribute__((pure)) f7(void*, int);
+
+void *ps[] = { f0, f1, f2, f3, f4, f5, f6, f7 };
 
 // CHECK: declare i32 @f0() [[RN:#[0-9]+]]
 // CHECK: declare i32 @f1() [[RO:#[0-9]+]]
@@ -22,6 +26,8 @@ void *ps[] = { f0, f1, f2, f3, f4, f5 };
 // CHECK: declare void @f3({{.*}} sret({{.*}}) align 4)
 // CHECK: declare void @f4({{.*}} byval({{.*}}) align 4)
 // CHECK: declare void @f5({{.*}} byval({{.*}}) align 4)
+// CHECK: declare void @f6({{.*}} sret({{.*}}) align 4, {{.*}} readnone{{.*}})
+// CHECK: declare void @f7({{.*}} sret({{.*}}) align 4, {{.*}} readonly{{.*}})
 
 // CHECK: attributes [[RN]] = { nounwind willreturn memory(none){{.*}} }
 // CHECK: attributes [[RO]] = { nounwind willreturn memory(read){{.*}} }

[efriedma-quic]

The general approach here seems okay.

[efriedma-quic]

Please CHECK the memory(argmem) marking (or however it's spelled).

I'm a little concerned about some of the {{.*}}; can you narrow them? Even if it's just going from sret({{.*}}) to sret({{[^)]*}}) or something... I'm concerned about an unexpected attribute hiding somewhere.

[qchateau]

I strengthened the expectations, including on f0 to f5. I changed the return type of f4/f5, the attributes const/pure were ignored because the functions' return type was void.

[efriedma-quic]

Looking through the kinds in clang/include/clang/CodeGen/CGFunctionInfo.h, I'm not sure Indirect is the only thing that needs to be handled specially.

I'd prefer to explicitly handle the cases you know are safe (Direct, Expand, CoerceAndExpand).

[qchateau]

Good call, it's also much simpler like this

[efriedma-quic]

LGTM

[efriedma-quic]

If you want me to merge for you, please let me know.

[qchateau]

Do I need to "Update branch" first ? Will it reset the approval ?
Anyway, you can merge it for me if it's faster than replying !

[mikaelholmen]

Hi @qchateau

gcc warns on the extra FunctionType = here that I suppose should be removed:

../llvm-project/clang/lib/CodeGen/CGCall.cpp:3021:53: warning: operation on 'FunctionType' may be undefined [-Wsequence-point]
 3021 |     llvm::FunctionType *FunctionType = FunctionType =
      |                                        ~~~~~~~~~~~~~^
 3022 |         getTypes().GetFunctionType(FI);
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~