[PATCH] D100689: [funcattrs] Consistent check call site attributes

[Philip Reames via Phabricator via llvm-commits]

reames created this revision.
reames added reviewers: jdoerfert, nlopes, nikic.
Herald added subscribers: hiraditya, mcrosier.
Herald added a reviewer: bollu.
reames requested review of this revision.
Herald added a project: LLVM.

This is mostly stylistic cleanup after D100226 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100689

Files:
  llvm/lib/Transforms/IPO/FunctionAttrs.cpp
  llvm/test/Transforms/FunctionAttrs/noreturn.ll
  llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll


Index: llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll
===================================================================
--- llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll
+++ llvm/test/Transforms/FunctionAttrs/willreturn-callsites.ll
@@ -1,4 +1,4 @@
-; 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
Index: llvm/test/Transforms/FunctionAttrs/noreturn.ll
===================================================================
--- llvm/test/Transforms/FunctionAttrs/noreturn.ll
+++ llvm/test/Transforms/FunctionAttrs/noreturn.ll
@@ -64,3 +64,10 @@
 define i32 @alreadynoreturn() noreturn {
   unreachable
 }
+
+; CHECK: Function Attrs: {{.*}}noreturn
+; CHECK-NEXT: @callsite_noreturn()
+define void @callsite_noreturn() {
+  call i32 @f() noreturn
+  ret void
+}
Index: llvm/lib/Transforms/IPO/FunctionAttrs.cpp
===================================================================
--- llvm/lib/Transforms/IPO/FunctionAttrs.cpp
+++ llvm/lib/Transforms/IPO/FunctionAttrs.cpp
@@ -1271,15 +1271,10 @@
   if (CB->hasFnAttr(Attribute::NoFree))
     return false;
 
-  Function *Callee = CB->getCalledFunction();
-  if (!Callee)
-    return true;
-
-  if (Callee->doesNotFreeMemory())
-    return false;
-
-  if (SCCNodes.contains(Callee))
-    return false;
+  // Speculatively assume in SCC.
+  if (Function *Callee = CB->getCalledFunction())
+    if (SCCNodes.contains(Callee))
+      return false;
 
   return true;
 }
@@ -1403,10 +1398,8 @@
 }
 
 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;
 }
 
@@ -1517,13 +1510,6 @@
   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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D100689.338247.patch
Type: text/x-patch
Size: 2483 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210416/5d63d6bb/attachment.bin>