[MergeFuncs] Fix callsite attributes in thunk generation

[• jrkoenig]

Summary

This change correctly sets the attributes on the callsites
generated in thunks. This makes sure things such as sret, sext, etc.
are correctly set, so that the call can be a proper tailcall.

Also, the transfer of attributes in the replaceDirectCallers function
appears to be unnecessary, but until this is confirmed it will remain.

[jfb]

lib/Transforms/IPO/MergeFunctions.cpp

1558 ↗

(On Diff #33894)

Why would non-null be missing? Could you add a test that shows this?

I'm not sure I get why the bitcast is needed here, and why you can't just drop the comment. It looks like it's there to match parameters/returns that are layout compatible but have different names. Why not cast those, instead of the function signature?

[• jrkoenig]

lib/Transforms/IPO/MergeFunctions.cpp

1558 ↗

(On Diff #33894)

There can be differences between attributes at a callsite and on a function, especially when a call was cross-TU but which is now a static callsite due to linking. The ABI attributes from a callsite and function must match (else the result is UB), and this must already have been true of the input because Old and New have the same ABI attributes. No ABI attributes are transferred in the test suite or in any of my LTO builds, but other attributes are. If transferring other attributes is valuable as an optimization then it should happen in it's own optimization pass, not here.

[jfb]

lib/Transforms/IPO/MergeFunctions.cpp

1558 ↗

(On Diff #33894)

Ah OK, that sounds unfortunate but not worth addressing in the patch. Could you clarify the comment with this?

[• jrkoenig]

Clarified comment about questionable attribute transfers.

[jfb]

lgtm

[dschuff]

lib/Transforms/IPO/MergeFunctions.cpp

1555 ↗

(On Diff #33968)

This comment is a bit confusing as-is because it's not clear what 'This' refers to. Presumably it's the paragraph below; should this paragraph go after the first one instead?

1563 ↗

(On Diff #33968)

'loose' -> lose

[• jrkoenig]

Fixed typo and comment order

llvm/trunk/lib/Transforms/IPO/MergeFunctions.cpp

 void MergeFunctions::replaceDirectCallers(Function *Old, Function *New) {
   Constant *BitcastNew = ConstantExpr::getBitCast(New, Old->getType());
   for (auto UI = Old->use_begin(), UE = Old->use_end(); UI != UE;) {
     Use *U = &*UI;
     ++UI;
     CallSite CS(U->getUser());
     if (CS && CS.isCallee(U)) {
       // Transfer the called function's attributes to the call site. Due to the
-      // bitcast we will 'loose' ABI changing attributes because the 'called
+      // bitcast we will 'lose' ABI changing attributes because the 'called
       // function' is no longer a Function* but the bitcast. Code that looks up
       // the attributes from the called function will fail.
+
+      // FIXME: This is not actually true, at least not anymore. The callsite
+      // will always have the same ABI affecting attributes as the callee,
+      // because otherwise the original input has UB. Note that Old and New
+      // always have matching ABI, so no attributes need to be changed.
+      // Transferring other attributes may help other optimizations, but that
+      // should be done uniformly and not in this ad-hoc way.
       auto &Context = New->getContext();
       auto NewFuncAttrs = New->getAttributes();
       auto CallSiteAttrs = CS.getAttributes();
 
       CallSiteAttrs = CallSiteAttrs.addAttributes(
           Context, AttributeSet::ReturnIndex, NewFuncAttrs.getRetAttributes());
 
       for (unsigned argIdx = 0; argIdx < CS.arg_size(); argIdx++) {
for (Function::arg_iterator AI = NewG->arg_begin(), AE = NewG->arg_end();
        AI != AE; ++AI) {
     Args.push_back(createCast(Builder, (Value*)AI, FFTy->getParamType(i)));
     ++i;
   }
 
   CallInst *CI = Builder.CreateCall(F, Args);
   CI->setTailCall();
   CI->setCallingConv(F->getCallingConv());
+  CI->setAttributes(F->getAttributes());
   if (NewG->getReturnType()->isVoidTy()) {
     Builder.CreateRetVoid();
   } else {
     Builder.CreateRet(createCast(Builder, CI, NewG->getReturnType()));
   }
 
   NewG->copyAttributesFrom(G);
   NewG->takeName(G);

llvm/trunk/test/Transforms/MergeFunc/apply_function_attributes.ll

define void @A(%Opaque_type* sret %a, %D2i* %b, i32* %xp, i32* %yp) {
   ret void
 }
 
 ; Make sure we transfer the parameter attributes to the call site.
 ; CHECK-LABEL: define void @C(%Opaque_type* sret
 ; CHECK:  tail call void bitcast (void (%Opaque_type*, %D2i*, i32*, i32*)* @A to void (%Opaque_type*, %S2i*, i32*, i32*)*)(%Opaque_type* sret %0, %S2i* %1, i32* %2, i32* %3)
 ; CHECK:  ret void
 
+
+; Make sure we transfer the parameter attributes to the call site.
+; CHECK-LABEL: define void @B(%Opaque_type* sret
+; CHECK:  %5 = bitcast
+; CHECK:  tail call void @A(%Opaque_type* sret %0, %D2i* %5, i32* %2, i32* %3)
+; CHECK:  ret void
+

llvm/trunk/test/Transforms/MergeFunc/inttoptr-address-space.ll

bb:
   ret i32 %tmp1
 }
 
 ; Check for pointer bitwidth equal assertion failure
 define internal i8* @func35(%.qux.2585 addrspace(1)* nocapture %this) align 2 {
 bb:
 ; CHECK-LABEL: @func35(
 ; CHECK: %[[V2:.+]] = bitcast %.qux.2585 addrspace(1)* %{{.*}} to %.qux.2496 addrspace(1)*
-; CHECK: %[[V3:.+]] = tail call i32 @func10(%.qux.2496 addrspace(1)* %[[V2]])
+; CHECK: %[[V3:.+]] = tail call i32 @func10(%.qux.2496 addrspace(1)* nocapture %[[V2]])
 ; CHECK: %{{.*}} = inttoptr i32 %[[V3]] to i8*
   %tmp = getelementptr inbounds %.qux.2585, %.qux.2585 addrspace(1)* %this, i32 0, i32 2
   %tmp1 = load i8*, i8* addrspace(1)* %tmp, align 4
   ret i8* %tmp1
 }

llvm/trunk/test/Transforms/MergeFunc/inttoptr.ll

bb:
   %tmp = getelementptr inbounds %.qux.2585, %.qux.2585* %this, i32 0
   ret i32* undef
 }
 
 define internal i8* @func35(%.qux.2585* nocapture %this) align 2 {
 bb:
 ; CHECK-LABEL: @func35(
 ; CHECK: %[[V2:.+]] = bitcast %.qux.2585* %{{.*}} to %.qux.2496*
-; CHECK: %[[V3:.+]] = tail call i32 @func10(%.qux.2496* %[[V2]])
+; CHECK: %[[V3:.+]] = tail call i32 @func10(%.qux.2496* nocapture %[[V2]])
 ; CHECK: %{{.*}} = inttoptr i32 %[[V3]] to i8*
   %tmp = getelementptr inbounds %.qux.2585, %.qux.2585* %this, i32 0, i32 2
   %tmp1 = load i8*, i8** %tmp, align 4
   ret i8* %tmp1
 }