[funcattrs] Consistently check call site attributes

[reames]

Summary

This is mostly stylistic cleanup after D100226, but not entirely. When skimming the code, I found one case where we weren't accounting for attributes on the callsite at all. I'm also suspicious we had some latent bugs related to operand bundles (which are supposed to be able to *override* attributes on declarations), but I don't have concrete test cases for those, just suspicions.

Aside: The only case left in the file which directly checks attributes on the declaration is the norecurse logic. I left that because I didn't understand it; it looks obviously wrong, so I suspect I'm misinterpreting the intended semantics of the attribute.

[nikic]

llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll

1

Why is the inferattrs here needed now?

[reames]

llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll

1

Because the declarations are only partially annotated, and func-attrs which is an CGSCC pass does not visit declarations, only inferattrs does.

[nikic]

LG

llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll

1

I'd personally prefer to add the missing nofree attributes to the declarations, so this only tests one pass. But I'm okay either way...

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

 static bool InstrBreaksNoFree(Instruction &I, const SCCNodeSet &SCCNodes) {
   CallBase *CB = dyn_cast<CallBase>(&I);
   if (!CB)
     return false;
 
   if (CB->hasFnAttr(Attribute::NoFree))
     return false;
 
-  Function *Callee = CB->getCalledFunction();
-  if (!Callee)
-    return true;
-
-  if (Callee->doesNotFreeMemory())
-    return false;
-
+  // Speculatively assume in SCC.
+  if (Function *Callee = CB->getCalledFunction())
     if (SCCNodes.contains(Callee))
       return false;
 
   return true;
 }
 
 /// Attempt to remove convergent function attribute when possible.
 ///
 /// Returns true if any changes to function attributes were made.
 static bool inferConvergent(const SCCNodeSet &SCCNodes) {
static bool addNoRecurseAttrs(const SCCNodeSet &SCCNodes) {
   // we have no indirect recursion as the SCC size is one. This function cannot
   // recurse.
   F->setDoesNotRecurse();
   ++NumNoRecurse;
   return true;
 }
 
 static bool instructionDoesNotReturn(Instruction &I) {
-  if (auto *CB = dyn_cast<CallBase>(&I)) {
-    Function *Callee = CB->getCalledFunction();
-    return Callee && Callee->doesNotReturn();
-  }
+  if (auto *CB = dyn_cast<CallBase>(&I))
+    return CB->hasFnAttr(Attribute::NoReturn);
   return false;
 }
 
 // A basic block can only return if it terminates with a ReturnInst and does not
 // contain calls to noreturn functions.
 static bool basicBlockCanReturn(BasicBlock &BB) {
   if (!isa<ReturnInst>(BB.getTerminator()))
     return false;
static bool InstrBreaksNoSync(Instruction &I, const SCCNodeSet &SCCNodes) {
   auto *CB = dyn_cast<CallBase>(&I);
   if (!CB)
     // Non call site cases covered by the two checks above
     return false;
 
   if (CB->hasFnAttr(Attribute::NoSync))
     return false;
 
-  // readnone + not convergent implies nosync
-  // (This is needed to initialize inference from declarations which aren't
-  //  explicitly nosync, but are readnone and not convergent.)
-  if (CB->hasFnAttr(Attribute::ReadNone) &&
-      !CB->hasFnAttr(Attribute::Convergent))
-    return false;
-
   // Non volatile memset/memcpy/memmoves are nosync
   // NOTE: Only intrinsics with volatile flags should be handled here.  All
   // others should be marked in Intrinsics.td.
   if (auto *MI = dyn_cast<MemIntrinsic>(&I))
     if (!MI->isVolatile())
       return false;
 
   // Speculatively assume in SCC.

llvm/test/Transforms/FunctionAttrs/noreturn.ll

define i32 @caller6() naked {
   ret i32 %c
 }
 
 ; CHECK: Function Attrs: {{.*}}noreturn
 ; CHECK-NEXT: @alreadynoreturn()
 define i32 @alreadynoreturn() noreturn {
   unreachable
 }
+
+; CHECK: Function Attrs: {{.*}}noreturn
+; CHECK-NEXT: @callsite_noreturn()
+define void @callsite_noreturn() {
+  call i32 @f() noreturn
+  ret void
+}

llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll

-; RUN: opt -function-attrs -S %s | FileCheck %s
+; RUN: opt -inferattrs -function-attrs -S %s | FileCheck %s
 
 declare void @decl_readonly() readonly
 declare void @decl_readnone() readnone
 declare void @decl_argmemonly(i32*) argmemonly
 declare void @decl_unknown()
 
 define void @test_fn_mustprogress(i32* %ptr) mustprogress {
 ; CHECK: Function Attrs: mustprogress