Skip to content

[PowerPC] Change half to use soft promotion rather than PromoteFloat #152632

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

tgross35
Copy link
Contributor

@tgross35 tgross35 commented Aug 8, 2025

On PowerPC targets, half uses the default legalization of promoting to
a f32. However, this has some fundamental issues related to inability
to round trip. Resolve this by switching to the soft legalization, which
passes f16 as an i16.

The PowerPC ABI Specification does not define a _Float16 type, so the
calling convention changes are acceptable.

Fixes the PowerPC part of #97975
Fixes the PowerPC part of #97981

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 8, 2025

The second commit is the interesting part here. The first commit is in a standalone PR at #152625 and should land first.

I am referencing https://ftp.rtems.org/pub/rtems/people/sebh/ABI64BitOpenPOWERv1.1_16July2015_pub.pdf for the ABI not having _Float16, not sure if there is a newer version available.

There is a new crash expanding LRINT that I am trying to figure out.

@Gelbpunkt
Copy link
Contributor

Gelbpunkt commented Aug 8, 2025

I am referencing https://ftp.rtems.org/pub/rtems/people/sebh/ABI64BitOpenPOWERv1.1_16July2015_pub.pdf for the ABI not having _Float16, not sure if there is a newer version available.

The latest specification of the ELFv2 ABI can be found here: https://files.openpower.foundation/s/cfA2oFPXbbZwEBK :)

"Table 2.13. Decimal Floating-Point Types" does not contain _Decimal16, so I think you are correct.

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 8, 2025

Oh that's great, thank you for the link!

"Table 2.13. Decimal Floating-Point Types" does not contain _Decimal16, so I think you are correct.

I think the correct place would be "Table 2.11. Scalar Types" under "Binary Floating-Point" (_Float16 is base-2, _Decimal16 is base-10), but the answer is the same.

@tgross35
Copy link
Contributor Author

tgross35 commented Aug 8, 2025

I'm stuck on the new crash, thought it just needed a new setOperationAction but that doesn't seem to do anything. Asked at https://discord.com/channels/636084430946959380/636732535434510338/1403227999050006621

@tgross35 tgross35 force-pushed the ppc-soft-promote-half branch 2 times, most recently from 35a405b to 77508b7 Compare August 8, 2025 12:45
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 8, 2025

@chenzheng1030, @EsmeYi, @lei137, @RolandF77 could you review this?

Only the last commit "[PowerPC] Change half to use soft promotion" is relevant to be reviewed for this PR. There are other commits here because it needs a few other things to land first:

I'm avoiding marking this as "ready to review" for now so it doesn't ping everybody from the backends touched in these other PRs, but the change to soft promotion here should be ready to review.

@tgross35 tgross35 force-pushed the ppc-soft-promote-half branch from 77508b7 to a8d24ba Compare August 8, 2025 13:16
@tgross35
Copy link
Contributor Author

tgross35 commented Aug 8, 2025

(current failure in "ERROR: test_modulelist_deadlock" has to be unrelated)

On platforms that soft promote `half`, using `lrint` intrinsics crashes
with the following:

    SoftPromoteHalfOperand Op #0: t5: i32 = lrint t4

    LLVM ERROR: Do not know how to soft promote this operator's operand!
    PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
    Stack dump:
    0.      Program arguments: /Users/tmgross/Documents/projects/llvm/llvm-build/bin/llc -mtriple=riscv32
    1.      Running pass 'Function Pass Manager' on module '<stdin>'.
    2.      Running pass 'RISC-V DAG->DAG Pattern Instruction Selection' on function '@test_lrint_ixx_f16'

Resolve this by adding a soft promotion.

`SoftPromoteHalfOp_FP_TO_XINT` is reused here since it provides the
correct input and output types. It is renamed `PromoteFloatOp_UnaryOp`
to match `PromoteFloatOp_UnaryOp` and similar functions that are used to
handle the same sets of intrinsics.
`f16` is more functional than just a storage type on the platform,
though it does have some codegen issues [1]. To prepare for future
changes, do the following nonfunctional updates to the existing `half`
test:

* Add tests for passing and returning the type directly.
* Add tests showing bitcast behavior, which is currently incorrect but
  serves as a baseline.
* Add tests for `fabs` and `copysign` (trivial operations that shouldn't
  require libcalls).
* Add invocations for big-endian and for PPC32.
* Rename the test to `half.ll` to reflect its status, which also matches
  other backends.

[1]: llvm#97975
On PowerPC targets, `half` uses the default legalization of promoting to
a `f32`. However, this has some fundamental issues related to inability
to round trip. Resolve this by switching to the soft legalization, which
passes `f16` as an `i16`.

The PowerPC ABI Specification does not define a `_Float16` type, so the
calling convention changes are acceptable.

Fixes the PowerPC portion of [1]. A similar change was done for MIPS in
f0231b6 ("[MIPS] Use softPromoteHalf legalization for fp16 rather
than PromoteFloat (llvm#110199)") and for Loongarch in 13280d9
("[loongarch][DAG][FREEZE] Fix crash when FREEZE a half(f16) type on
loongarch (llvm#107791)").

[1]: llvm#97975
@tgross35 tgross35 force-pushed the ppc-soft-promote-half branch from a8d24ba to dcecedd Compare August 12, 2025 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants