This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add intrinsic to convert between ppc_fp128 and fp128
ClosedPublic

Authored by qiucf on Sep 8 2021, 12:54 AM.

Details

Summary

ppc_fp128 and fp128 are both 128-bit floating point types. However, we can't do conversion between them now. (trunc/ext are not allowed for same-size fp types)

This patch adds two new intrinsics: llvm.ppc.convert.f128.to.ppcf128 and llvm.convert.ppcf128.to.f128, to support such conversion.

Diff Detail

Event Timeline

qiucf created this revision.Sep 8 2021, 12:54 AM
qiucf requested review of this revision.Sep 8 2021, 12:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2021, 12:54 AM
qiucf added a reviewer: jsji.
lkail edited reviewers, added: efriedma; removed: eli.friedman.Sep 8 2021, 2:09 AM

This could be a powerpc-specific intrinsic, maybe? I guess that basically just shoving around the code a bit, but maybe clearer to readers. The name is also a little ambiguous: it's not clear from the name that the target type must be fp128.

Long-term, we might want to consider replacing fpext/fptrunc instructions with something like fpconv. We now have two pairs of floating-point types that have the same size, and if we ever add more floating-point types, the problems will just get more complicated.

llvm/lib/IR/Function.cpp
1031 ↗(On Diff #371269)

Please define a new IITDescriptorKind for this.

qiucf updated this revision to Diff 372411.Sep 13 2021, 11:49 PM
qiucf marked an inline comment as done.
qiucf retitled this revision from [SDAG] Add intrinsic to convert between ppc_fp128 and fp128 to [PowerPC] Add intrinsic to convert between ppc_fp128 and fp128.
qiucf edited the summary of this revision. (Show Details)

Make it a PowerPC intrinsic.

Thanks for doing this. Looks reasonable to me. Some code format comments.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10390

Lint warnings.

Do we need a local variable for LC?

11031

Can we put this also in LowerINTRINSIC_WO_CHAIN? and call LowerINTRINSIC_WO_CHAIN here? So we can have a concentrated place for the ppc128/f128 conversion?

11039

no need for local Op?

qiucf updated this revision to Diff 382389.Oct 26 2021, 10:27 AM
qiucf marked 3 inline comments as done.
shchenz accepted this revision.Oct 26 2021, 6:53 PM

LGTM. Thanks.

Better to wait for several days in case other reviewers have comments.

This revision is now accepted and ready to land.Oct 26 2021, 6:53 PM
qiucf updated this revision to Diff 384989.Nov 5 2021, 1:58 AM

Rebase and fix typo

This revision was landed with ongoing or failed builds.Nov 5 2021, 2:02 AM
This revision was automatically updated to reflect the committed changes.