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

[Denis Antrushin via Phabricator via llvm-commits]

dantrushin marked an inline comment as done.
dantrushin added a comment.

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

> Second, there appears to be a semantic problem around the handling of base vs derived slots unless we *always* spill the base.  We can't tie both uses to a single def.  This may warrant some offline discussion.


Yes, we can't. But apparently, we do not need to.
Basically, that tied-ness information is only needed to glue two live ranges into one during SSA flattening. And it does not matter how many uses end live range at the same instruction.
Luckily, LLVM appears to handle it properly.

But if included tests (and checks) do not make you believe it works, we should throw all this stuff away and try some different approach.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp:225
   unsigned NumVRegs = HasVRegVariadicDefs ? NumResults : II.getNumDefs();
+  if (Node->getMachineOpcode() == TargetOpcode::STATEPOINT)
+    NumVRegs = NumResults;
----------------
reames wrote:
> dantrushin wrote:
> > reames wrote:
> > > This line looks unnecessary provided the defs list is marked variadic.  If you want to leave it for the moment, that's fine, but I think we can clean this up in a follow up change.
> > > 
> > > (To be specific, OutOperandList in Target.td for STATEPOINT can be variable_ops, and this line disappears.)
> > `II` here is `MCInstrDesc`.
> > Statepoint has 0 defs in it MCIntrDesc. (It is `MachineInstr::getNumDefs` which work correctly for variadic outs).
> > So I really need it here
> > 
> Please read the line above your change with the explicit handling for variadic defs.
> 
> (Not a blocking item.  I'll fix after submit if needed)
First, please notice `!MF->getTarget().usesPhysRegsForValues()` there.
```
  /// True if the target uses physical regs (as nearly all targets do). False
  /// for stack machines such as WebAssembly and other virtual-register
  /// machines. If true, all vregs must be allocated before PEI. If false, then
  /// callee-save register spilling and scavenging are not needed or used. If
  /// false, implicitly defined registers will still be assumed to be physical
  /// registers, except that variadic defs will be allocated vregs.
  virtual bool usesPhysRegsForValues() const { return true; }
```
This function returns `true` for all targets except WebAssembly, so `HasVRegVariadicDefs` is 
always `false` on e.g., X86

Second,  looks like `variadicOpsAreDefs` as it was implemented for ARM does not means exactly what I wanted.
Looking at `MCInstrDesc::hasDefOfPhysReg` I see that variadic defs are supposed to go after static uses.
Even though this would work for current definition of STATEPOINT - because it have no static uses 
(and this is somewhat questionable from my POW) - I want STATEPOINT defs to come first, just like in all
ordinary instructions.
WebAssembly abused this attribute for its own purposes, but I did not want to.
In fact, for our purposes we can compute `isVariadicDefs` property same way as `isVariadic` is computed,
without any need for additional attributes. I'm not sure if `isVariadicDefs` would work for WebAssembly/ARM
so I decided not to dig into this until we have first variadic-defs instruction in tree.





Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81648