[PATCH] D81648: MIR Statepoint refactoring. Part 4: ISEL changes.

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

I've spent several hours today wrapping my head around this patch.  I think I've found a material simplification which should greatly simplify the code.

You track the def offset for each pointer of interest.  You have plumbed through the merge value code with the purpose of being able to get to the SDNode corresponding to the actual statepoint.  In the gc.relocate code, you combine these two pieces into enough information to extract the relevant definition from the statepoint.  Under the assumption that we're only talking about relocates within a single basic block, this is correct.

My suggested simplification is the following.  Rather than maintaining a map to offsets in the STATEPOINT node, simply maintain a map from pointer to SDValue representing the particular output of the node.  Doing this means that you do not need any of the merge value code, all of the result propagation remains the same, and the gc.relocate changes are isolated.  You would need to export these values in the cross block case, but this is fairly easily handled by copying each output to a vreg during lowering of the statepoint and having the vreg be the value tracked in the FuncInfo.

If my written description is not clear, or you disagree with my conclusions, let's find a time to talk offline.

In addition to this larger change, I have added a couple of smaller FIXMEs throughout the day.  I've also been landing NFCs where doing so made sense in the process of understanding your changes.  You should expect a non-trivial, but not particularly difficult either, rebase.

You should consider this comment a blocking comment.  I do not intend to spend further time on this review until either a) the changes noted today have been made, or b) we've talked offline and we've agreed to an alternate approach.



================
Comment at: llvm/include/llvm/CodeGen/FunctionLoweringInfo.h:106
+  DenseMap<const Instruction *, DerivedPtrMapTy> DerivedPtrMap;
+
   /// StaticAllocaMap - Keep track of frame indices for fixed sized allocas in
----------------
Marker (see overall comment)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81648