[PATCH] D71173: [MergeFuncs] Remove incorrect attribute copying

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: jfb, whitequark, vsk.
Herald added subscribers: llvm-commits, dexonsmith, hiraditya, kristof.beyls.
Herald added a project: LLVM.
nikic edited the summary of this revision.

Fix for https://bugs.llvm.org/show_bug.cgi?id=44236. This code was originally introduced in rG36512330041201e10f5429361bbd79b1afac1ea1 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71173

Files:
  llvm/lib/Transforms/IPO/MergeFunctions.cpp
  llvm/test/Transforms/MergeFunc/byval-attr-congruent-type.ll


Index: llvm/test/Transforms/MergeFunc/byval-attr-congruent-type.ll
===================================================================
--- /dev/null
+++ 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
+}
Index: llvm/lib/Transforms/IPO/MergeFunctions.cpp
===================================================================
--- llvm/lib/Transforms/IPO/MergeFunctions.cpp
+++ llvm/lib/Transforms/IPO/MergeFunctions.cpp
@@ -450,28 +450,10 @@
     ++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);
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D71173.232730.patch
Type: text/x-patch
Size: 3001 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20191208/9c6bb252/attachment.bin>