[InstCombine / BuildLibCalls] Add parameter attributes from the prototype.

[jonpa]

Summary

Currently only the calling convention is passed on from the prototype to the built call instruction. This patch also copies the parameter attributes.

This seems to be needed after D101806, which changed the instruction selector to only look at the call instruction parameter attributes, whereas before it also looked at the called function.

There are probably more places that needs a similar fix, like builtins..?

This makes the "verifier extension" (D103412) pass on SPEC17/SystemZ. Not sure if the returned value is checked by that patch or if they need a similar handling like the parameters?

[aeubanks]

I reverted that change and probably won't revisit it soon due to all these sorts of issues.
But this change looks good on its own

llvm/test/Transforms/InstCombine/libcall-param-attrs.ll

2

is this necessary?

[rnk]

+1

llvm/test/Transforms/InstCombine/libcall-param-attrs.ll

2

You should be able to pass -mtriple to make this test work.

[jonpa]

Thanks for review!

This was not quite complete to commit as it was a suggestion and there were still the tests updates to do...

I could update most of the tests with update_test_checks.py, and most of the remaining manually (not quite sure if the function attribute '#0' tag should be part of the test line or not, but I think it shouldn't matter).

However, there are now two remaining tests that the verifier rejects, but I am not quite sure how to fix them:

LLVM :: Transforms/InstCombine/strcpy_chk-1.ll
LLVM :: Transforms/InstCombine/strncpy_c

Instcombiner builds the memcpy, but this time the 'returned' attribute is included as a parameter attribute. That makes Verifier.cpp (line 1962) do the check (which it simply omitted before)

Assert(Ty->canLosslesslyBitCastTo(FT->getReturnType())

where Ty is '*i8', and FT->getReturnType() is 'void'.

There doesn't really seem to be an error as the call looks like

call void @llvm.memcpy.p0i8.p0i8.i32(i8* noalias noundef nonnull returned ...

I suppose getReturnType() should in this case return '*i8' and not void. Or possibly the Verifier could be adapted to understand this check?

Given that the problematic patch was reverted, I wonder now if this is still relevant and needed and we should fix the above problem now?

[rnk]

I think the verifier is doing the right thing, I think it's a bug that @llvm.memcpy is carrying a returned attribute on the first argument. I'm not sure how that is happening, but whatever sets that up seems like a bug.

[jonpa]

In D103992#2814596, @rnk wrote:

I think the verifier is doing the right thing, I think it's a bug that @llvm.memcpy is carrying a returned attribute on the first argument. I'm not sure how that is happening, but whatever sets that up seems like a bug.

That was added by 8715e03 / https://reviews.llvm.org/D51092. I tried reverting that and got some test failures, whereof one in DeadStoreElimination, so it seems these are useful in some way...

@efriedma: You have any comment on how to proceed here (given you reviewed that revision)?

[efriedma]

D51092 is marking the libc memcpy function, which is correct. Not sure how the attribute is getting carried over to llvm.memcpy.

[jonpa]

D51092 is marking the libc memcpy function, which is correct. Not sure how the attribute is getting carried over to llvm.memcpy.

Ah, I see. This is what happened:

call i8* @__strcpy_chk

=> FortifiedLibCallSimplifier::optimizeStrpCpyChk =>

call i8* @strcpy(i8* noalias returned writeonly getelementptr inbounds...

=> optimizeStringMemoryLibCall() => optimizeStrCpy() => CreateMemCpy => 

*NewCI = CreateMemTransferInst(Intrinsic::memcpy);
NewCI->setAttributes(CI->getAttributes());

So the llvm.memcpy ends up taking the attributes from the created strcpy...

To remedy this, I made a new helper function takeAttributesFrom() that makes sure that attributes transferred onto llvm.memcpy has the Returned attribute removed if present. With that, the tests are passing.

Does this seem right, and if so, I suppose this should be done with the other intrinsics as well, e.g. after CreateMemSet()?

[efriedma]

Taking all the attributes from a call to one function and applying them to a call to a different function doesn't make sense to me. Different functions have different attributes.

There might be a few specific attributes that make sense to transfer, but you'd want to explicitly handle the specific attributes that are relevant. (For example, pointer alignment.)

[jonpa]

In D103992#2820880, @efriedma wrote:

Taking all the attributes from a call to one function and applying them to a call to a different function doesn't make sense to me. Different functions have different attributes.

There might be a few specific attributes that make sense to transfer, but you'd want to explicitly handle the specific attributes that are relevant. (For example, pointer alignment.)

It seems that all callers of CreateMemCpy() so far has assumed that taking the attributes of the original call is the right thing to do. The only change here is to not take the Returned attribute.

If this is incorrect, I suppose I should not create the helper function and just remove the Returned attribute where needed for now?

[efriedma]

Can we just completely skip transferring the attributes to the memcpy?

[rnk]

I agree, we probably need to better understand what attributes we are trying to transfer. My guess is that we are trying to transfer call site attributes, things like noinline, alwaysinline, or other things inferred by optimization. If the call's prototype changes, then copying argument attributes doesn't make any sense and we should re-evaluate doing it in the first place.

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

1222

This change is fine: we are taking the attributes from a known callee.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

199 ↗

(On Diff #352286)

This seems questionable. Why not take the parameter attributes from NewCI's callee? What is the value of merging in the attributes from the old call?

[jonpa]

Can we just completely skip transferring the attributes to the memcpy?

I removed takeAttributesFrom() and this seemed to not make much of a difference at all, at least not on SPEC17/SystemZ (NFC). Some InstCombine tests changed though - not sure if that look ok or not...

There are also other calls, such as CreateMemSet()/CreateMemMove() that still take all the attributes in the same way from the original CallInst... Maybe these should be removed now as well, or are they different?

I see in LibCallSimplifier::optimizeStrNCpy() a transfer of the parameter attributes only, which may make sense though...

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

199 ↗

(On Diff #352286)

Not sure if there could be any value of taking parameter attributes from the old call... It seems though that the call to CreateMemCpy already passes on explicit attributes (alignment), so I suppose that might be enough to do in that place?

Is there anything similar to inferLibFuncAttributes() for these intrinsics that we should use?

[rnk]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1120–1121 ↗

(On Diff #353238)

This change makes this comment stale. I checked using blame, and this setAttributes call was added in rGe80fcf03407acd6429d07e4a45185ac546ffa37c by @xbolva00 to propagate the nonnull annotations. However, I don't see any changes to the tests that were added, so maybe we don't need this any longer. Maybe the attributes are present on the memcpy declaration already.

In conclusion, I think the change looks good, just remove this stale comment. @xbolva00 , feel free to check that logic.

[jonpa]

Stale comment removed

[jonpa]

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1120–1121 ↗

(On Diff #353238)

Removed (not sure about leaving the 'TODO'..?)

[rnk]

lgtm, go ahead and remove the TODO before landing

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp

1120–1121 ↗

(On Diff #353238)

The TODO was added in D95088, which you have removed the need for. I believe it can also be removed.

[aeubanks]

This is somehow adding the returned attribute to a memcpy intrinsic on mac: https://crbug.com/1223647.
I'll dig deeper tomorrow into what's going wrong

llvm/lib/Transforms/Utils/BuildLibCalls.cpp

 
 //- Emit LibCalls ------------------------------------------------------------//
 
 Value *llvm::castToCStr(Value *V, IRBuilderBase &B) {
   unsigned AS = V->getType()->getPointerAddressSpace();
   return B.CreateBitCast(V, B.getInt8PtrTy(AS), "cstr");
 }
 
+static void setCallingConvAndAttrs(CallInst *&CI, const Value *V) {
+  if (const Function *F = dyn_cast<Function>(V)) {
+    CI->setCallingConv(F->getCallingConv());
+    CI->setAttributes(F->getAttributes());
+  }
+}
+
 static Value *emitLibCall(LibFunc TheLibFunc, Type *ReturnType,
                           ArrayRef<Type *> ParamTypes,
                           ArrayRef<Value *> Operands, IRBuilderBase &B,
                           const TargetLibraryInfo *TLI,
                           bool IsVaArgs = false) {
   if (!TLI->has(TheLibFunc))
     return nullptr;
 
   Module *M = B.GetInsertBlock()->getModule();
   StringRef FuncName = TLI->getName(TheLibFunc);
   FunctionType *FuncType = FunctionType::get(ReturnType, ParamTypes, IsVaArgs);
   FunctionCallee Callee = M->getOrInsertFunction(FuncName, FuncType);
   inferLibFuncAttributes(M, FuncName, *TLI);
   CallInst *CI = B.CreateCall(Callee, Operands, FuncName);
-  if (const Function *F =
-          dyn_cast<Function>(Callee.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
+  setCallingConvAndAttrs(CI, Callee.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitStrLen(Value *Ptr, IRBuilderBase &B, const DataLayout &DL,
                         const TargetLibraryInfo *TLI) {
   LLVMContext &Context = B.GetInsertBlock()->getContext();
   return emitLibCall(LibFunc_strlen, DL.getIntPtrType(Context),
                      B.getInt8PtrTy(), castToCStr(Ptr, B), B, TLI);
Value *llvm::emitMemCpyChk(Value *Dst, Value *Src, Value *Len, Value *ObjSize,
   LLVMContext &Context = B.GetInsertBlock()->getContext();
   FunctionCallee MemCpy = M->getOrInsertFunction(
       "__memcpy_chk", AttributeList::get(M->getContext(), AS), B.getInt8PtrTy(),
       B.getInt8PtrTy(), B.getInt8PtrTy(), DL.getIntPtrType(Context),
       DL.getIntPtrType(Context));
   Dst = castToCStr(Dst, B);
   Src = castToCStr(Src, B);
   CallInst *CI = B.CreateCall(MemCpy, {Dst, Src, Len, ObjSize});
-  if (const Function *F =
-          dyn_cast<Function>(MemCpy.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
+  setCallingConvAndAttrs(CI, MemCpy.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitMemPCpy(Value *Dst, Value *Src, Value *Len, IRBuilderBase &B,
                          const DataLayout &DL, const TargetLibraryInfo *TLI) {
   LLVMContext &Context = B.GetInsertBlock()->getContext();
   return emitLibCall(
       LibFunc_mempcpy, B.getInt8PtrTy(),
static Value *emitUnaryFloatFnCallHelper(Value *Op, StringRef Name,
   CallInst *CI = B.CreateCall(Callee, Op, Name);
 
   // The incoming attribute set may have come from a speculatable intrinsic, but
   // is being replaced with a library call which is not allowed to be
   // speculatable.
   CI->setAttributes(Attrs.removeAttribute(B.getContext(),
                                           AttributeList::FunctionIndex,
                                           Attribute::Speculatable));
-  if (const Function *F =
-          dyn_cast<Function>(Callee.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
+  setCallingConvAndAttrs(CI, Callee.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitUnaryFloatFnCall(Value *Op, StringRef Name, IRBuilderBase &B,
                                   const AttributeList &Attrs) {
   SmallString<20> NameBuffer;
   appendTypeSuffix(Op, Name, NameBuffer);
 
static Value *emitBinaryFloatFnCallHelper(Value *Op1, Value *Op2,
   CallInst *CI = B.CreateCall(Callee, { Op1, Op2 }, Name);
 
   // The incoming attribute set may have come from a speculatable intrinsic, but
   // is being replaced with a library call which is not allowed to be
   // speculatable.
   CI->setAttributes(Attrs.removeAttribute(B.getContext(),
                                           AttributeList::FunctionIndex,
                                           Attribute::Speculatable));
-  if (const Function *F =
-          dyn_cast<Function>(Callee.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
+  setCallingConvAndAttrs(CI, Callee.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitBinaryFloatFnCall(Value *Op1, Value *Op2, StringRef Name,
                                    IRBuilderBase &B,
                                    const AttributeList &Attrs) {
   assert((Name != "") && "Must specify Name to emitBinaryFloatFnCall");
 
Value *llvm::emitPutChar(Value *Char, IRBuilderBase &B,
   inferLibFuncAttributes(M, PutCharName, *TLI);
   CallInst *CI = B.CreateCall(PutChar,
                               B.CreateIntCast(Char,
                               B.getInt32Ty(),
                               /*isSigned*/true,
                               "chari"),
                               PutCharName);
 
-  if (const Function *F =
-          dyn_cast<Function>(PutChar.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
+  setCallingConvAndAttrs(CI, PutChar.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitPutS(Value *Str, IRBuilderBase &B,
                       const TargetLibraryInfo *TLI) {
   if (!TLI->has(LibFunc_puts))
     return nullptr;
 
   Module *M = B.GetInsertBlock()->getModule();
   StringRef PutsName = TLI->getName(LibFunc_puts);
   FunctionCallee PutS =
       M->getOrInsertFunction(PutsName, B.getInt32Ty(), B.getInt8PtrTy());
   inferLibFuncAttributes(M, PutsName, *TLI);
   CallInst *CI = B.CreateCall(PutS, castToCStr(Str, B), PutsName);
-  if (const Function *F =
-          dyn_cast<Function>(PutS.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
+  setCallingConvAndAttrs(CI, PutS.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitFPutC(Value *Char, Value *File, IRBuilderBase &B,
                        const TargetLibraryInfo *TLI) {
   if (!TLI->has(LibFunc_fputc))
     return nullptr;
 
   Module *M = B.GetInsertBlock()->getModule();
   StringRef FPutcName = TLI->getName(LibFunc_fputc);
   FunctionCallee F = M->getOrInsertFunction(FPutcName, B.getInt32Ty(),
                                             B.getInt32Ty(), File->getType());
   if (File->getType()->isPointerTy())
     inferLibFuncAttributes(M, FPutcName, *TLI);
   Char = B.CreateIntCast(Char, B.getInt32Ty(), /*isSigned*/true,
                          "chari");
   CallInst *CI = B.CreateCall(F, {Char, File}, FPutcName);
-
-  if (const Function *Fn =
-          dyn_cast<Function>(F.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(Fn->getCallingConv());
+  setCallingConvAndAttrs(CI, F.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitFPutS(Value *Str, Value *File, IRBuilderBase &B,
                        const TargetLibraryInfo *TLI) {
   if (!TLI->has(LibFunc_fputs))
     return nullptr;
 
   Module *M = B.GetInsertBlock()->getModule();
   StringRef FPutsName = TLI->getName(LibFunc_fputs);
   FunctionCallee F = M->getOrInsertFunction(FPutsName, B.getInt32Ty(),
                                             B.getInt8PtrTy(), File->getType());
   if (File->getType()->isPointerTy())
     inferLibFuncAttributes(M, FPutsName, *TLI);
   CallInst *CI = B.CreateCall(F, {castToCStr(Str, B), File}, FPutsName);
-
-  if (const Function *Fn =
-          dyn_cast<Function>(F.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(Fn->getCallingConv());
+  setCallingConvAndAttrs(CI, F.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitFWrite(Value *Ptr, Value *Size, Value *File, IRBuilderBase &B,
                         const DataLayout &DL, const TargetLibraryInfo *TLI) {
   if (!TLI->has(LibFunc_fwrite))
     return nullptr;
 
   Module *M = B.GetInsertBlock()->getModule();
   LLVMContext &Context = B.GetInsertBlock()->getContext();
   StringRef FWriteName = TLI->getName(LibFunc_fwrite);
   FunctionCallee F = M->getOrInsertFunction(
       FWriteName, DL.getIntPtrType(Context), B.getInt8PtrTy(),
       DL.getIntPtrType(Context), DL.getIntPtrType(Context), File->getType());
 
   if (File->getType()->isPointerTy())
     inferLibFuncAttributes(M, FWriteName, *TLI);
   CallInst *CI =
       B.CreateCall(F, {castToCStr(Ptr, B), Size,
                        ConstantInt::get(DL.getIntPtrType(Context), 1), File});
-
-  if (const Function *Fn =
-          dyn_cast<Function>(F.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(Fn->getCallingConv());
+  setCallingConvAndAttrs(CI, F.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitMalloc(Value *Num, IRBuilderBase &B, const DataLayout &DL,
                         const TargetLibraryInfo *TLI) {
   if (!TLI->has(LibFunc_malloc))
     return nullptr;
 
   Module *M = B.GetInsertBlock()->getModule();
   StringRef MallocName = TLI->getName(LibFunc_malloc);
   LLVMContext &Context = B.GetInsertBlock()->getContext();
   FunctionCallee Malloc = M->getOrInsertFunction(MallocName, B.getInt8PtrTy(),
                                                  DL.getIntPtrType(Context));
   inferLibFuncAttributes(M, MallocName, *TLI);
   CallInst *CI = B.CreateCall(Malloc, Num, MallocName);
-
-  if (const Function *F =
-          dyn_cast<Function>(Malloc.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
+  setCallingConvAndAttrs(CI, Malloc.getCallee()->stripPointerCasts());
   return CI;
 }
 
 Value *llvm::emitCalloc(Value *Num, Value *Size, const AttributeList &Attrs,
                         IRBuilderBase &B, const TargetLibraryInfo &TLI) {
   if (!TLI.has(LibFunc_calloc))
     return nullptr;
 
   Module *M = B.GetInsertBlock()->getModule();
   StringRef CallocName = TLI.getName(LibFunc_calloc);
   const DataLayout &DL = M->getDataLayout();
   IntegerType *PtrType = DL.getIntPtrType((B.GetInsertBlock()->getContext()));
   FunctionCallee Calloc = M->getOrInsertFunction(
       CallocName, Attrs, B.getInt8PtrTy(), PtrType, PtrType);
   inferLibFuncAttributes(M, CallocName, TLI);
   CallInst *CI = B.CreateCall(Calloc, {Num, Size}, CallocName);
-
-  if (const auto *F =
-          dyn_cast<Function>(Calloc.getCallee()->stripPointerCasts()))
-    CI->setCallingConv(F->getCallingConv());
-
+  setCallingConvAndAttrs(CI, Calloc.getCallee()->stripPointerCasts());
   return CI;
 }

llvm/test/Transforms/InstCombine/libcall-param-attrs.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+; REQUIRES: systemz-registered-target
+;
+; Check that instcombiner creates libcalls with parameter extensions per the
+; prototype.
+
+declare dso_local i8* @strchr(i8*, i32) local_unnamed_addr #1
+declare dso_local i8* @memchr(i8*, i32 signext, i64)
+declare void @llvm.assume(i1 noundef)
+@0 = private unnamed_addr constant [21 x i8] c"000000000000000000000", align 2
+define void @fun0(i32 %arg1) {
+; CHECK: define void @fun0
+; CHECK: call i8* @memchr{{.*}}, i32 signext %arg1, i64 22)
+bb:
+  %i = call i8* @strchr(i8* getelementptr inbounds ([21 x i8], [21 x i8]* @0, i64 0, i64 0), i32 signext %arg1)
+  %i3 = icmp ne i8* %i, null
+  call void @llvm.assume(i1 %i3)
+  ret void
+}
+
+declare dso_local double @pow(double, double)
+define void @fun1(i32* %i5) {
+; CHECK: define void @fun1
+; CHECK: call double @ldexp{{.*}}, i32 signext %i19)
+bb:
+  %i19 = load i32, i32* %i5, align 4
+  %i20 = sitofp i32 %i19 to double
+  %i21 = call double @pow(double 2.000000e+00, double %i20)
+  ret void
+}