[MergeFuncs] Remove incorrect attribute copying

[nikic]

Summary

Fix for https://bugs.llvm.org/show_bug.cgi?id=44236. This code was originally introduced in rG36512330041201e10f5429361bbd79b1afac1ea1. However, the attribute copying was done in the wrong place (in general call replacement, not thunk generation) and a proper fix was implemented in D12581.

Previously this code was just unnecessary but harmless (because FunctionComparator ensured that the attributes of the two functions are exactly the same), but since byval was changed to accept a type this copying is actively wrong and may result in malformed IR.

[vsk]

Thanks, lgtm.

llvm/lib/Transforms/IPO/MergeFunctions.cpp

 // Replace direct callers of Old with New.
 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 '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 NewPAL = New->getAttributes();
-      SmallVector<AttributeSet, 4> NewArgAttrs;
-      for (unsigned argIdx = 0; argIdx < CS.arg_size(); argIdx++)
-        NewArgAttrs.push_back(NewPAL.getParamAttributes(argIdx));
-      // Don't transfer attributes from the function to the callee. Function
-      // attributes typically aren't relevant to the calling convention or ABI.
-      CS.setAttributes(AttributeList::get(Context, /*FnAttrs=*/AttributeSet(),
-                                          NewPAL.getRetAttributes(),
-                                          NewArgAttrs));
-
+      // Do not copy attributes from the called function to the call-site.
+      // Function comparison ensures that the attributes are the same up to
+      // type congruences in byval(), in which case we need to keep the byval
+      // type of the call-site, not the callee function.
       remove(CS.getInstruction()->getFunction());
       U->set(BitcastNew);
     }
   }
 }
 
 // Helper for writeThunk,
 // Selects proper bitcast operation,

llvm/test/Transforms/MergeFunc/byval-attr-congruent-type.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -mergefunc %s | FileCheck %s
+
+%struct.c = type { i32 }
+%struct.a = type { i32 }
+
+@d = external dso_local global %struct.c
+
+define void @e(%struct.a* byval(%struct.a) %f) {
+; CHECK-LABEL: @e(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+define void @g(%struct.c* byval(%struct.c) %f) {
+; CHECK-LABEL: @g(
+; CHECK-NEXT:    ret void
+;
+  ret void
+}
+
+define void @h() {
+; CHECK-LABEL: @h(
+; CHECK-NEXT:    call void bitcast (void (%struct.a*)* @e to void (%struct.c*)*)(%struct.c* byval(%struct.c) @d)
+; CHECK-NEXT:    ret void
+;
+  call void @g(%struct.c* byval(%struct.c) @d)
+  ret void
+}