[PATCH] D139209: [IRMover] Remove UB implying parameter attributes when necessary

[Tim Neumann via Phabricator via llvm-commits]

TimNN added inline comments.


================
Comment at: llvm/lib/Linker/IRMover.cpp:1554
   /// it has no effect on definitions in the same module.
-  if (auto *F = dyn_cast<Function>(&GV)) {
-    if (!F->isIntrinsic())
-      F->removeFnAttr(llvm::Attribute::NoCallback);
+  if (DstF) {
+    if (!DstF->isIntrinsic())
----------------
tejohnson wrote:
> How about just do an early return if !DstF, since your changes below also rely on DstF being non-null.
Given the generic name ("updateAttributes") and purpose of this function, I decided to avoid any early returns. 

(Put differently: This function currently has two mostly independent (and long) sections, each with their own prerequisites. I wanted to keep the control-flow contained to the individual sections, especially in light of the possibility that this function may handle non-function globals in the future).

I'd be happy to add the early return, but in that case I'd advocate for renaming the function "update**Function**Attributes".


================
Comment at: llvm/lib/Linker/IRMover.cpp:1568
+  // If `F` was a declaration in its source module but is (already) a
+  // definition in the destination module, then the signatures of the two
+  // functions may have diverged (even if they were originally idential) due to
----------------
tejohnson wrote:
> I think this is backwards? My understanding (also from the code and the test case) is that the issue occurs if F is a declaration in the dest module but was a definition in the source module.
Yes, of course. If I don't pay very close attention it's too easy to mix up the two. I've updated the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D139209/new/

https://reviews.llvm.org/D139209