[PATCH] D98884: [IR] Ignore bitcasts of function pointers which are only used as callees in callbase instruction

[Madhur Amilkanthwar via Phabricator via llvm-commits]

madhur13490 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2135-2139
+  CallBase *CB = dyn_cast<CallBase>(U);
+  if (!CB) {
+    assert(false && "Expected CallBase");
+    return;
+  }
----------------
jdoerfert wrote:
> arsenm wrote:
> > Just use cast<>
> This can at some point error out. Why would the user of `ChangeCalleesToFastCall` and others need to require this property?
I think I am missing something here. This function is capturing common logic used by `ChangeCalleesToFastCall`.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2156
   }
 }
 
----------------
jdoerfert wrote:
> This part was not in the original patch and is now tagged along. At the very least add it to the commit message. It is also unclear why you chose these two functions, are there no other users() loops?
This fix is needed because GlobalOpt/evaluate-call-errors.ll failed with an assert as now OptimizeFunctions() is making a call to this function because of the change in return value by hasAddressTaken(). 

Apologies, I should have handled this earlier.



================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2185
+    }
+    SetCallBaseAttributes(U, F, A);
   }
----------------
jdoerfert wrote:
> The function is called remove... and the helper set...
Well, the `RemoveAttribute` is indeed setting attributes by making call to setAttributes() and the utility function is just wrapping the commonly used code. Should we rename the function? 


================
Comment at: llvm/test/Analysis/CallGraph/ignore-bitcast-call-argument-callee.ll:12
+; CHECK-NEXT:  CS<{{.*}}> calls function 'bar'
+; CHECK-NEXT:  CS<{{.*}}> calls external node
+
----------------
jdoerfert wrote:
> I run this with trunk and the output looks the same. What exactly is this testing?
Yeah, output is same. But the test case shows that hasAddressTaken() return value is indeed correct when there is a direct call as well as bitcast callees is used as an argument as is used by call graph builder. The similar tests in Analysis/CallGraph,check only the call graph. 


================
Comment at: llvm/test/Transforms/GlobalOpt/bitcast-callees-fastcc.ll:17
+  ret void
+}
+
----------------
jdoerfert wrote:
> Also unclear what is tested here? Looks like the trunk output.
This test demonstrates that the amendments made in this patch would actually set "fastcc" on bitcast callees. With trunk, "fastcc" would not be set on "foo" because hasAddressTaken() is true when queried by OptimizeFunctions() in GlobalOpt.cpp



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98884