Page MenuHomePhabricator

[PowerPC] Support lowering int-to-fp on ppc_fp128

Authored by qiucf on Jun 16 2020, 2:26 AM.


Group Reviewers
Restricted Project

D70867 introduced support for expanding most ppc_fp128 operations. But sitofp/uitofp is missing. This patch adds that after D81669.

Note: There's still issue about chain for ppc_fp128 on SPE subtargets. But I'm not sure if the type is fully supported on SPE.

Diff Detail

Event Timeline

qiucf created this revision.Jun 16 2020, 2:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2020, 2:26 AM

For unsigned conversion, the input is treated as signed here, and the result fixed up by adding 2^N; something along the lines of

(signed)N >= 0 ? signed_to_FP((signed)N) : signed_to_FP(N - 2^N) + (FP)2^N

For strict operation, we need to verify that the result is correct with any setting of the current rounding mode, and that the overall exception status after the two operations (signed_to_FP and the FADD) matches the intended semantics of the unsigned_to_FP operation. I believe in general this is not the case; specifically, there may be problems if the input (interpreted as signed) is not exactly representable as FP number,

If the input type is i32 or i64, that's probably not a problem, given that all such numbers are exactly representable as ppc_f128. However, if the input type is i128, this is no longer the case.

If you look at SelectionDAGLegalize::ExpandLegalINT_TO_FP, which uses a similar algorithm, you'll see that it switches to a different one in the case where not all inputs may be representable. I think we'll have to do something similar here for full correctness.

(OTOH given that this affects only the i128->ppc_f128 case, which should be relatively rare, and I think isn't even fully correct in all cases in non-strict mode even today, maybe this can be done later. But at least there should be a FIXME.)

qiucf updated this revision to Diff 278692.Jul 17 2020, 2:53 AM

Add FIXME for i128->ppc_fp128 strict semantics fix.

qiucf updated this revision to Diff 278693.Jul 17 2020, 2:54 AM
This comment was removed by qiucf.
qiucf updated this revision to Diff 278695.Jul 17 2020, 2:56 AM

Revert last update. Put the right revision back...

uweigand requested changes to this revision.Jul 17 2020, 3:43 AM

See inline comments about chain handling.


This looks like the wrong input Chain if we already performed an operation above -- it should use the output chain of that operation as input here.


OK, well, even if we keep the existing algorithm with the FIXME, this still isn't correct as it loses the Chain after the FADD. I think in the strict case we'll need to use a strict version of SelectCC (passing in the Chain from the FADD) and the use the resulting output Chain for the overall output (via ReplaceValueWith as above -- in fact, if we fall through down here, we actually should *not* do the ReplaceValueWith above, since that would be the wrong chain).

This revision now requires changes to proceed.Jul 17 2020, 3:43 AM
qiucf added inline comments.Aug 13 2020, 1:16 AM

Seems we don't have strict select_cc. Here Src is integer type, not suitable for fsetcc+select?

And yes, I should replace original chain of N with newest chain, both here and inside isSigned if block.

uweigand added inline comments.Aug 13 2020, 6:00 AM

Ah, you're right -- this is an integer select, so there is no (need for a) strict version.

So the only change needed is to do the chain replacement here, and inside the isSigned block (as you said).

qiucf updated this revision to Diff 286745.Aug 20 2020, 1:58 AM

Adjust expanded chain

uweigand accepted this revision.Aug 20 2020, 8:34 AM

LGTM now. Thanks!

This revision is now accepted and ready to land.Aug 20 2020, 8:34 AM
qiucf closed this revision.Aug 23 2020, 10:25 PM