[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.

Denis, I'm having a really hard time wrapping my head around this code, even knowing what it is supposed to do.  We need to clean this up a bit; it's not in a state where I can approve it.

A couple of suggestions for you:

1. Since some of the complexity here seems specific to invokes, let's restrict the first patch to call instructions.  If we see an invoke, we can simply use the stack lowering.
2. The handling around needing the first N GC values in a sorted order seems overly complicated.  I was also unsure about the correctness of derived pointer handling when base != derived.  What would you think of explicitly keeping track of which operands we've created that are tied def/use pairs?  We could visit operands in the current order and then use the stack lowering when the size of the defs list is larger than our threshold.
3. Testing wise, I think it make sense to start with a new test file, build out a set of tests which exercise this, and only update all of the tests once we have something which is expected to work end to end.  We're not there yet.





================
Comment at: llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp:70
+cl::opt<bool> UseRegistersForGCPointers(
+    "use-registers-for-gcptrs", cl::Hidden, cl::init(false),
+    cl::desc("Allow using registers for GC pointer meta args"));
----------------
For consistency with the above, please call this "-use-registers-for-gc-values".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81648