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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8068
+    DAG.ReplaceAllUsesOfValueWith(Op, Mov);
+    return Conv;
+  } else {
----------------
This looks still wrong to me.  I think you should replace the *chain* with the one from Conv, and return the value (i.e. Mov).  Something like:

SDValue Mov = DAG.getNode(PPCISD::MFVSR, dl, Op.getValueType(), Conv);
DAG.ReplaceAllUsesOfValueWith(SDValue(Op, 1), Conv.getValue(1));
return Mov;

(Actually, returning the value is then the same in strict and non-strict cases, so this can be merged.)

Or, another possible approach would be to not use ReplaceAllUses at all but reconstruct the multi-output value via   DAG.getMergeValues.   Something like;

  SDValue Mov = DAG.getNode(PPCISD::MFVSR, dl, Op.getValueType(), Conv);
  return DAG.getMergeValues({Mov, Conv.getValue(1)}, dl);



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrVSX.td:3772
+def : Pat<(i32 (any_fp_to_uint f128:$src)),
           (i32 (MFVSRWZ (COPY_TO_REGCLASS (XSCVQPUWZ $src), VFRC)))>;
 
----------------
This also doesn't look quite correct.  The XSCVQP... instructions are not (yet?) marked as mayRaiseFPException, instead they're marked as hasSideEffects.   This means that the exception flag is probably not going to be automatically transferred over to the MI level.

I think if the instructions are changed to set mayRaiseFPException, that should work correctly.  But it would be best to have a test case that validates that the "nofpexcept" marker is transferred depending on the value of the "fpexect." metadata in the strict intrinsic (in LLVM IR).


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

https://reviews.llvm.org/D81537