[llvm] b361d3b - [MergeFuncs] Remove incorrect attribute copying

[Nikita Popov via llvm-commits]

Author: Nikita Popov
Date: 2019-12-11T20:09:54+01:00
New Revision: b361d3bbcd85647c9f6640a5f57932c43fdb7a1a

URL: https://github.com/llvm/llvm-project/commit/b361d3bbcd85647c9f6640a5f57932c43fdb7a1a
DIFF: https://github.com/llvm/llvm-project/commit/b361d3bbcd85647c9f6640a5f57932c43fdb7a1a.diff

LOG: [MergeFuncs] Remove incorrect attribute copying

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.

Differential Revision: https://reviews.llvm.org/D71173

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

Modified: 
    llvm/lib/Transforms/IPO/MergeFunctions.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/IPO/MergeFunctions.cpp b/llvm/lib/Transforms/IPO/MergeFunctions.cpp
index 27dd1030dc57..a702de03e46a 100644
--- a/llvm/lib/Transforms/IPO/MergeFunctions.cpp
+++ b/llvm/lib/Transforms/IPO/MergeFunctions.cpp
@@ -450,28 +450,10 @@ void MergeFunctions::replaceDirectCallers(Function *Old, Function *New) {
     ++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);
     }

diff  --git a/llvm/test/Transforms/MergeFunc/byval-attr-congruent-type.ll b/llvm/test/Transforms/MergeFunc/byval-attr-congruent-type.ll
new file mode 100644
index 000000000000..7e7d772b977d
--- /dev/null
+++ b/llvm/test/Transforms/MergeFunc/byval-attr-congruent-type.ll
@@ -0,0 +1,30 @@
+; 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 }
+
+ at 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
+}