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

Mon Jun 22 08:35:22 PDT 2020

dantrushin added a comment.

In D81648#2104721 <https://reviews.llvm.org/D81648#2104721>, @reames wrote:

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

Are you talking about that cross-block usage (CopyToRegs/CopyFromRegs) stuff?
All this complexity is already there for `gc.result`. 
For `gc.relocate` I do exactly the same, only I have to use separate map, because `ValueMap` is occupied by `gc.result` 
Some of the complexities was required to handle undef/duplicate `gc.relocate` cases. Maybe now, when we have `undef` handling upstream, I'll be able to simplify it a bit

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

I can track them all the way through lowering, yes (in `FunctionLoweringInfo`), but the problem is that I need to tie registers in InstrEmitter. Since `STATEPOINT`  does not have meaningful `MachineDescription`, I need to get this information from somewhere else.
I cannot store that mapping in `SDNode`, and `InstrEmitter` does not have access to `SelectionDAG` or `FunctionLoweringInfo`, so I have to figure it out from the statepoint `SDNode` itself.
//"First N"// approach is the only one I can think of.
I can map gc values to registers in order until I encounter something 'non-register' and move that sorting stuff to say, `CodeGenPrepare`. Does it sounds better?
Could you clarify why you're worrying about `base!=derived` case? Base pointers are not interesting at all - they all can be assigned to registers (no need to tie them). It is `base==derived` case which is interesting/complicated. I've been worrying about LLVM's
ability to handle to identical uses only one of which is tied to def, but apparently it works.

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

Could you explain? This change is last one in series and completes the whole sequence. What's missing here?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81648