[llvm] [MemProf] Don't mutate the function type when calling clone (PR #147829)

[Teresa Johnson via llvm-commits]

================
@@ -5173,7 +5173,13 @@ bool MemProfContextDisambiguation::applyImport(Module &M) {
           CBClone = CB;
         else
           CBClone = cast<CallBase>((*VMaps[J - 1])[CB]);
-        CBClone->setCalledFunction(NewF);
+        // Set the called operand directly instead of calling setCalledFunction,
+        // as the latter mutates the function type on the call. In rare cases
+        // we may have a slightly different type on a callee function
+        // declaration due to it being imported from a different module with
+        // incomplete types. We really just want to change the name of the
+        // function to the clone, and not make any type changes.
+        CBClone->setCalledOperand(NewF.getCallee());
----------------
teresajohnson wrote:

I started to add such an assert, however, it seems unnecessary to me as we create NewF from the function type of CalledFunction just a few lines above here. However, looking at this made me concerned about the possibility of a different issue. Where we get CalledFunction we try various ways to find the function from the CallBase if getCalledFunction returns null. One of those things is to invoke stripPointerCasts. Now if we had a pointer cast here it would not be valid (pre-opaque pointers) to simply replace the called operand. But in theory, with opaque pointers, we should not actually find any pointer casts to strip. For testing, I added some checks in that code, as well as in the module summary analysis which does the same thing for all ThinLTO bitcode generation and kicks in much more broadly, and never found (with the regression tests or with a large app) any where it found anything to actually strip. But out of an abundance of caution I have added some handling above here to make sure the called function is the called operand (or its aliasee), and skip rewriting the call otherwise (also incrementing a new statistic in case we want to watch for cases this actually doing anything). In theory we could remove the stripPointerCasts call, but that needs to stay in sync with module summary analysis, so that's a change that could be considered later.

https://github.com/llvm/llvm-project/pull/147829