[PATCH] [Inliner][NFCI] Add an InlineSite abstraction.

[Sanjoy Das]

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:66
@@ -65,1 +65,3 @@
 namespace {
+class InlineSite {
+  Function *CalledFunction;
----------------
reames wrote:
> I'm not convinced of the need for a new abstraction here.  What does this really add over having a function which takes a call site and extracts the underlying callee from a statepoint and the surface callee from a normal call?  An getUnwrappedCallee(CallSite CS) function seems to serve the same purpose with much less complexity.
> 
> Even if I accept the need for an extra abstraction, I *strongly* object to the duplication of the underlying fields.  Add proxying access if needed, but please do not duplicate each field from the underlying call site.
> What does this really add over having a function which takes a call site and extracts the underlying callee from a statepoint and the surface callee from a normal call?

It is not just the call destination -- I will have to update all of the places where the Inliner accesses, say, paramHasAttr and change it to do something different if the call site is a statepoint.  I suppose I could have a `getUnwrappedXXX` for each of those properties, but that's semantically what this class is.  Plus by having a separate type I can verify that I've not accidentally used `CS.getFoo()` where it would be incorrect to do so.

> Even if I accept the need for an extra abstraction, I *strongly* object to the duplication of the underlying fields. Add proxying access if needed, but please do not duplicate each field from the underlying call site.

Ok.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:131
@@ +130,3 @@
+
+  Instruction *TheCall = CS.getInstruction();
+  if (!TheCall->use_empty()) {
----------------
reames wrote:
> I'm confused by this bit.  It seems like possibly an unrelated refactoring?  Or is this the statepoint related bits for gc.result?
I moved this part so to that I could RAUW `gc_result` easily in the following patch.  I'll split this bit out into its own refactoring.

================
Comment at: lib/Transforms/Utils/InlineFunction.cpp:1367
@@ -1264,13 +1366,3 @@
 
-    // If the return instruction returned a value, replace uses of the call with
-    // uses of the returned value.
-    if (!TheCall->use_empty()) {
-      ReturnInst *R = Returns[0];
-      if (TheCall == R->getReturnValue())
-        TheCall->replaceAllUsesWith(UndefValue::get(TheCall->getType()));
-      else
-        TheCall->replaceAllUsesWith(R->getReturnValue());
-    }
-    // Since we are now done with the Call/Invoke, we can delete it.
-    TheCall->eraseFromParent();
+    ReturnVal = Returns[0]->getReturnValue();
 
----------------
reames wrote:
> Unintentional diff?
See previous comment.

http://reviews.llvm.org/D10631

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/