[PATCH] Support of the gc.relocates for invoke statepoints

[Sanjoy Das]

I think this is mostly ready to go in.  Let's do one more iteration if you don't mind.

Btw, some of the `GCRelocateOperands` accessors you use have been renamed; but those will show up as compiler errors.


REPOSITORY
  rL LLVM

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:79
@@ +78,3 @@
+  // used across basic block boundaries.
+  // First value of the key is statepoint, second is value for which location
+  // is stored.
----------------
What is "location" here?  Is it the frame index?

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:83
@@ +82,3 @@
+  // but didn't spill it.
+  typedef DenseMap<std::pair<const Instruction*, const Value*>, Optional<int>>
+    StatepointRelocatedValuesMap;
----------------
I'd find this easier to understand if you had `DenseMap<Instruction *, DenseMap<Value *, int>>` instead.  IOW, currently your map is of the type `(Statept, Value) -> Index`, but I think making it of the type `Statept -> Value -> Index` would be clearer (even though it is equivalent) as it would emphasize that //per safepoint// we have a mapping of `Value`s to frame indices.

================
Comment at: include/llvm/CodeGen/FunctionLoweringInfo.h:85
@@ +84,3 @@
+    StatepointRelocatedValuesMap;
+  StatepointRelocatedValuesMap StatepointRelocatedValues;
+
----------------
I'm not sure if the `typedef` is adding much here.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:540
@@ +539,3 @@
+  // Record computed locations for all lowered values.
+  // This can not be embeded in lowering loops as we need to record *all*
+  // values, while previous loops account only values with unique SDValues.
----------------
Minor: should be `embedded`.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:542
@@ +541,3 @@
+  // values, while previous loops account only values with unique SDValues.
+  for (GCRelocateOperands relocateOpers :
+         StatepointSite.getRelocates(StatepointSite)) {
----------------
Use `RelocatedOpers` or `RO` as the variable name.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:545
@@ +544,3 @@
+    const Value *V = relocateOpers.derivedPtr();
+    SDValue SdV = Builder.getValue(V);
+    SDValue Loc = Builder.StatepointLowering.getLocation(SdV);
----------------
LLVM style would be `SDV` here.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:553
@@ +552,3 @@
+    }
+    else {
+      // Record value as visited, but not spilled. This is case for allocas
----------------
I think LLVM style is `} else {`.  You might want to just run this patch through `clang-format` before check-in.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:557
@@ +556,3 @@
+      // visiting corresponding gc_relocate.
+      // Actually we may not record them in this map at all. We do this only
+      // to check that we are not relocating any unvisited value.
----------------
I'd change the language slightly:  "Actually we do not need to record them in this map at all." assuming that is what you meant.

================
Comment at: lib/CodeGen/SelectionDAG/StatepointLowering.cpp:730
@@ -694,1 +729,3 @@
 void SelectionDAGBuilder::visitGCRelocate(const CallInst &CI) {
+  GCRelocateOperands relocateOpers(&CI);
+
----------------
Use `RelocateOpers` or `RO`.

http://reviews.llvm.org/D7798

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