[AMDGPU] Fix discarded result of addAttribute

[modocache]

Summary

llvm::AttributeList and llvm::AttributeSet are immutable, and so methods
defined on these classes, such as addAttribute, return a new immutable
object with the attribute added. In https://reviews.llvm.org/D55217 I attempted
to annotate methods such as addAttribute with LLVM_NODISCARD, since
calling these methods has no side-effects, and so ignoring the result
that is returned is almost certainly a programmer error.

However, committing the change resulted in new warnings in the AMDGPU target.
The AMDGPU simplify libcalls pass added in https://reviews.llvm.org/D36436
attempts to add the readonly and nounwind attributes to simplified
library functions, but instead calls the addAttribute methods and
ignores the result.

Modify the simplify libcalls pass to actually add the nounwind and
readonly attributes. Also update the simplify libcalls test to assert
that these attributes are actually being set.

[nhaehnle]

test/CodeGen/AMDGPU/simplify-libcalls.ll

791 ↗

(On Diff #177210)

Shouldn't this be #[[$NOUNWIND_READONLY]] = { nounwind readonly }? Or better yet, drop the $ - I really don't know why the existing checks have $NOUNWIND instead of just NOUNWIND.

[modocache]

Thanks for the review, @nhaehnle! I changed the FileCheck variable NOUNWIND_READONLY to not use a dollar-sign prefix. I'm not sure why the other lines in this test do, but I left them alone for now. Any other suggestions for this, or is it good to go?

[arsenm]

The $ is supposed to be for check variables that are used globally and not cleared on a -LABEL

[modocache]

test/CodeGen/AMDGPU/simplify-libcalls.ll

790 ↗

(On Diff #177427)

My original diff fixed the typo in this check statement -- this should be GCN-PRELINK, not CGN-PRELINK. But in the interest of keeping this patch small and only changing one thing at a time, I've left this line alone for now. For what it's worth, the tests do pass with this typo fixed so that the CHECK is actually run.

[arsenm]

In D55435#1324753, @arsenm wrote:

The $ is supposed to be for check variables that are used globally and not cleared on a -LABEL

i.e. it's correct for this use case where the check is outside of the scope of the defining -LABEL block

[modocache]

I see, thanks @arsenm! I think this variable fits those criteria so I changed it back to using a dollar-sign prefix. Variable names notwithstanding, does the implementation change seem OK to everyone? It fixes what I believe is clearly a programming error. Landing this change will unblock me from adding LLVM_NODISCARD to these functions and so preventing another case of this same error.

[rampitec]

LGTM

llvm/trunk/lib/Target/AMDGPU/AMDGPULibFunc.cpp

Function *AMDGPULibFunc::getOrInsertFunction(Module *M,
 
   Constant *C = nullptr;
   if (hasPtr) {
     // Do not set extra attributes for functions with pointer arguments.
     C = M->getOrInsertFunction(FuncName, FuncTy);
   } else {
     AttributeList Attr;
     LLVMContext &Ctx = M->getContext();
-    Attr.addAttribute(Ctx, AttributeList::FunctionIndex, Attribute::ReadOnly);
-    Attr.addAttribute(Ctx, AttributeList::FunctionIndex, Attribute::NoUnwind);
+    Attr = Attr.addAttribute(Ctx, AttributeList::FunctionIndex,
+                             Attribute::ReadOnly);
+    Attr = Attr.addAttribute(Ctx, AttributeList::FunctionIndex,
+                             Attribute::NoUnwind);
     C = M->getOrInsertFunction(FuncName, FuncTy, Attr);
   }
 
   return cast<Function>(C);
 }
 
 bool UnmangledFuncInfo::lookup(StringRef Name, ID &Id) {
   auto Loc = Map.find(Name);

llvm/trunk/test/CodeGen/AMDGPU/simplify-libcalls.ll

entry:
   %tmp21 = addrspacecast i8 addrspace(1)* %tmp20 to i8*
   %tmp22 = tail call i32 @__read_pipe_2(%opencl.pipe_t addrspace(1)* %p128, i8* %tmp21, i32 128, i32 128) #0
   %tmp23 = bitcast %struct.S addrspace(1)* %ptru to i8 addrspace(1)*
   %tmp24 = addrspacecast i8 addrspace(1)* %tmp23 to i8*
   %tmp25 = tail call i32 @__read_pipe_2(%opencl.pipe_t addrspace(1)* %pu, i8* %tmp24, i32 400, i32 4) #0
   ret void
 }
 
+; GCN-PRELINK: declare float @_Z4fabsf(float) local_unnamed_addr #[[$NOUNWIND_READONLY:[0-9]+]]
+; GCN-PRELINK: declare float @_Z4cbrtf(float) local_unnamed_addr #[[$NOUNWIND_READONLY]]
+; GCN-PRELINK: declare float @_Z11native_sqrtf(float) local_unnamed_addr #[[$NOUNWIND_READONLY]]
+
 ; CGN-PRELINK: attributes #[[$NOUNWIND]] = { nounwind }
+; GCN-PRELINK: attributes #[[$NOUNWIND_READONLY]] = { nounwind readonly }
 attributes #0 = { nounwind }