[PATCH] D86605: [PowerPC] Expand constrained ppc_fp128 to i32 conversion

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:2970
   def FADDrtz: PPCCustomInserterPseudo<(outs f8rc:$FRT), (ins f8rc:$FRA, f8rc:$FRB), "",
-                      [(set f64:$FRT, (PPCfaddrtz f64:$FRA, f64:$FRB))]>;
+                      [(set f64:$FRT, (PPCany_faddrtz f64:$FRA, f64:$FRB))]>;
 }
----------------
qiucf wrote:
> uweigand wrote:
> > You should verify the FADDrtz custom inserter works correctly for the strict case.  I believe it should transfer the no-exception flag to the FADD it generates in the end.
> Thanks for pointing out this. Actually, I see here `FADD` is always NOT `nofpexcept` unless transfer this flag from `FADDRtz` to it.
> 
> Besides, I notice if we use `!metadata "fpexcept.ignore`, `FADDRtz` doesn't obtain `nofpexcept` flag. That's because we dropped SD flags in lowering these nodes while they're target strict opcodes, so seen as //may raise exception//.
> 
> Similar flag missing is also seen on X86 (not tested other targets), a constrained `fptoui` from double to i64 would be lowered to `fptosi`. The flag missed during this.
Hmm, interesting.  I agree the NoFPExcept flag should be transfered over.  I'm not actually sure it is correct to transfer *all* the flags to all the operations.  There's actually a comment wondering about that:
    // TODO: Should any fast-math-flags be set for the FSUB?

Maybe it would be safer for now just to transfer the one flag?


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

https://reviews.llvm.org/D86605