[PATCH] D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

In D81537#2207249 <https://reviews.llvm.org/D81537#2207249>, @qiucf wrote:

> In D81537#2193216 <https://reviews.llvm.org/D81537#2193216>, @uweigand wrote:
>
>> This doesn't look correct.  As far as I can see, none of the conversion functions were actually changed to handle strict operations.  For one, you'll need strict variants of all the PowerPC-specific conversion operations, use them in all the conversion subroutines, and consistently track their chain nodes.
>>
>> The patch only adds a strict variant of the direct move, which seems to me the only operation where actually a strict version is **not** required ...
>
> Thanks for pointing them out! I have something unclear about chains:
>
> (1) If a constrained operation is expanded into several FP nodes `a-b-c`, they should all have chain set to former operation (`b`'s chain is `a`, `c`'s chain is `b`) even if they have def relationship?

That may depend on the specific semantics on which of those nodes may or may not trap.  In some cases, the original sequence may in fact not be valid at all for strict mode.  But if it is, then they'll need to be chained up properly.  If they have data dependencies, then it usually makes sense for the chain to follow that dependency.  In other cases, the may be an option for more flexibility by allowing certain operations to be re-scheduled.  In those cases you'd give the same input chain to all operations and collect all output chains via a TokenFactor.

> (2) In `MachineInstr` emitting after ISel, chains are identified just by operand type (countOperand <https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/SelectionDAG/InstrEmitter.cpp#L62-L68>), so some chains are not ignored and assert hit. Is this expected?

I'm not sure I understand what specific case you're refering to.   But in any case, a chain should *never* be simply ignored, that would always be a bug.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8307
+    return DAG.getNode(PPCISD::MFVSR, dl, Op.getValueType(),
+                       convertFPToInt(Op, DAG, Subtarget));
 }
----------------
qiucf wrote:
> uweigand wrote:
> > Why do we need a strict version of a plain move?
> Yes, a strict move doesn't look reasonable. But the original `strict_fptosi` node will be replaced by the result. So if directly return the move, operands will not match (no chain in `mfvsr`). Is there a better way here?
So this gets expanded to a PPCISD::FCTI... variant (inside convertFPToInt) followed by the MFVSR.  Now, with proper chain handling, the input chain of the strict_fptosi is consumed by a strict variant of FCTI..., and the output chain of that STRICT_FCTI... is then the correct output chain for the whole operation.  The data output (only) of the STRICT_FCTI... acts then as the input of the MFVSR, and the output of the MFVSR is the correct value output of the whole operation.

So if short, you need to replace
  (out-val, out-chain) = strict_fptosi (in-val, in-chain)
by
  (tmp-val, out-chain) = STRICT_FCTI... (in-val, in-chain)
  out-val = MFVSR (tmp-val)

This probably will require some ReplaceAllUses... instead of just returning a result, as is already done elsewhere with chain output instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81537