[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat #110199

yingopq · 2024-09-27T02:41:07Z

Fix part of #97975.

github-actions · 2024-09-27T02:44:32Z

✅ With the latest revision this PR passed the C/C++ code formatter.

nikic

Fix #97975.

This only fixes it for the MIPS architecture, so should not close the issue with "Fix".

The fix itself here looks fine to me, but I feel like the test coverage is very weak. There's just that one test, and because it's not a generated one, it omits too much information to even tell if the assembly is correct.

nikic · 2024-09-27T07:14:31Z

llvm/lib/Target/Mips/MipsISelLowering.h

-      SDValue Lo = DAG.getNode(MipsISD::Lo, DL, Ty,
-                               getTargetNode(N, Ty, DAG, LoFlag));
+      SDValue Lo =
+          DAG.getNode(MipsISD::Lo, DL, Ty, getTargetNode(N, Ty, DAG, LoFlag));


Unrelated reformat? Please use git-clang-format to format PRs, it should leave unrelated lines alone.

I had used the git clang-format tool before submitting this PR, and it automatically modified the irrelevant code format, so I submitted it together.

I would do it locally to check it again.

yingopq · 2024-09-27T07:31:25Z

Fix #97975.

This only fixes it for the MIPS architecture, so should not close the issue with "Fix".

The fix itself here looks fine to me, but I feel like the test coverage is very weak. There's just that one test, and because it's not a generated one, it omits too much information to even tell if the assembly is correct.

I would update the title and enhance the test file.

nikic · 2024-10-01T12:49:47Z

llvm/test/CodeGen/Mips/fp16-promote.ll

Can you please rebase over 9f81acf?

OK, updated.

@nikic Please help review, thanks!

yingopq · 2024-10-30T02:57:29Z

Ping

tgross35 · 2024-11-05T09:11:53Z

Is it possible to add a test with to_half and from_half from #97981? The codegen be asm-only with no libcalls. Currently __gnu_h2f_ieee / __gnu_f2h_ieee get called https://llvm.godbolt.org/z/M81EsbMbG.

For #110199.

…loat Fix part of llvm#97975.

nikic · 2024-11-05T09:20:40Z

@tgross35 Done.

nikic

LGTM

For llvm#110199.

…loat (llvm#110199) Fix part of llvm#97975.

…onger cause a crash (#116569) This PR fixes a bug introduced by #110199, which causes any half float argument to crash the compiler on MIPS64. Currently compiling this bit of code with `llc -mtriple=mips64`: ``` define void @half_args(half %a) nounwind { entry: ret void } ``` Crashes with the following log: ``` LLVM ERROR: unable to allocate function argument #0 PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. Stack dump: 0. Program arguments: llc -mtriple=mips64 1. Running pass 'Function Pass Manager' on module '<stdin>'. 2. Running pass 'MIPS DAG->DAG Pattern Instruction Selection' on function '@half_args' #0 0x000055a3a4013df8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x32d0df8) #1 0x000055a3a401199e llvm::sys::RunSignalHandlers() (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x32ce99e) #2 0x000055a3a40144a8 SignalHandler(int) Signals.cpp:0:0 #3 0x00007f00bde558c0 __restore_rt libc_sigaction.c:0:0 #4 0x00007f00bdea462c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76 #5 0x00007f00bde55822 gsignal ./signal/../sysdeps/posix/raise.c:27:6 #6 0x00007f00bde3e4af abort ./stdlib/abort.c:81:7 #7 0x000055a3a3f80e3c llvm::report_fatal_error(llvm::Twine const&, bool) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x323de3c) #8 0x000055a3a2e20dfa (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x20dddfa) #9 0x000055a3a2a34e20 llvm::MipsTargetLowering::LowerFormalArguments(llvm::SDValue, unsigned int, bool, llvm::SmallVectorImpl<llvm::ISD::InputArg> const&, llvm::SDLoc const&, llvm::SelectionDAG&, llvm::SmallVectorImpl<llvm::SDValue>&) const MipsISelLowering.cpp:0:0 #10 0x000055a3a3d896a9 llvm::SelectionDAGISel::LowerArguments(llvm::Function const&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30466a9) #11 0x000055a3a3e0b3ec llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c83ec) #12 0x000055a3a3e09e21 llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c6e21) #13 0x000055a3a2aae1ca llvm::MipsDAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) MipsISelDAGToDAG.cpp:0:0 #14 0x000055a3a3e07706 llvm::SelectionDAGISelLegacy::runOnMachineFunction(llvm::MachineFunction&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c4706) #15 0x000055a3a3051ed6 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x230eed6) #16 0x000055a3a35a3ec9 llvm::FPPassManager::runOnFunction(llvm::Function&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x2860ec9) #17 0x000055a3a35ac3b2 llvm::FPPassManager::runOnModule(llvm::Module&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x28693b2) #18 0x000055a3a35a499c llvm::legacy::PassManagerImpl::run(llvm::Module&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x286199c) #19 0x000055a3a262abbb main (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x18e7bbb) #20 0x00007f00bde3fc4c __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3 #21 0x00007f00bde3fd05 call_init ./csu/../csu/libc-start.c:128:20 #22 0x00007f00bde3fd05 __libc_start_main@GLIBC_2.2.5 ./csu/../csu/libc-start.c:347:5 #23 0x000055a3a2624921 _start /builddir/glibc-2.39/csu/../sysdeps/x86_64/start.S:117:0 ``` This is caused by the fact that after the change, `f16`s are no longer lowered as `f32`s in calls. Two possible fixes are available: - Update calling conventions to properly support passing `f16` as integers. - Update `useFPRegsForHalfType()` to return `true` so that `f16` are still kept in `f32` registers, as before #110199. This PR implements the first solution to not introduce any more ABI changes as #110199 already did. As of what is the correct ABI for halfs, I don't think there is a correct answer. GCC doesn't support halfs on MIPS, and I couldn't find any information on old MIPS ABI manuals either.

…onger cause a crash (llvm#116569) This PR fixes a bug introduced by llvm#110199, which causes any half float argument to crash the compiler on MIPS64. Currently compiling this bit of code with `llc -mtriple=mips64`: ``` define void @half_args(half %a) nounwind { entry: ret void } ``` Crashes with the following log: ``` LLVM ERROR: unable to allocate function argument #0 PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace. Stack dump: 0. Program arguments: llc -mtriple=mips64 1. Running pass 'Function Pass Manager' on module '<stdin>'. 2. Running pass 'MIPS DAG->DAG Pattern Instruction Selection' on function '@half_args' #0 0x000055a3a4013df8 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x32d0df8) #1 0x000055a3a401199e llvm::sys::RunSignalHandlers() (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x32ce99e) llvm#2 0x000055a3a40144a8 SignalHandler(int) Signals.cpp:0:0 llvm#3 0x00007f00bde558c0 __restore_rt libc_sigaction.c:0:0 llvm#4 0x00007f00bdea462c __pthread_kill_implementation ./nptl/pthread_kill.c:44:76 llvm#5 0x00007f00bde55822 gsignal ./signal/../sysdeps/posix/raise.c:27:6 llvm#6 0x00007f00bde3e4af abort ./stdlib/abort.c:81:7 llvm#7 0x000055a3a3f80e3c llvm::report_fatal_error(llvm::Twine const&, bool) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x323de3c) llvm#8 0x000055a3a2e20dfa (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x20dddfa) llvm#9 0x000055a3a2a34e20 llvm::MipsTargetLowering::LowerFormalArguments(llvm::SDValue, unsigned int, bool, llvm::SmallVectorImpl<llvm::ISD::InputArg> const&, llvm::SDLoc const&, llvm::SelectionDAG&, llvm::SmallVectorImpl<llvm::SDValue>&) const MipsISelLowering.cpp:0:0 llvm#10 0x000055a3a3d896a9 llvm::SelectionDAGISel::LowerArguments(llvm::Function const&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30466a9) llvm#11 0x000055a3a3e0b3ec llvm::SelectionDAGISel::SelectAllBasicBlocks(llvm::Function const&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c83ec) llvm#12 0x000055a3a3e09e21 llvm::SelectionDAGISel::runOnMachineFunction(llvm::MachineFunction&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c6e21) llvm#13 0x000055a3a2aae1ca llvm::MipsDAGToDAGISel::runOnMachineFunction(llvm::MachineFunction&) MipsISelDAGToDAG.cpp:0:0 llvm#14 0x000055a3a3e07706 llvm::SelectionDAGISelLegacy::runOnMachineFunction(llvm::MachineFunction&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x30c4706) llvm#15 0x000055a3a3051ed6 llvm::MachineFunctionPass::runOnFunction(llvm::Function&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x230eed6) llvm#16 0x000055a3a35a3ec9 llvm::FPPassManager::runOnFunction(llvm::Function&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x2860ec9) llvm#17 0x000055a3a35ac3b2 llvm::FPPassManager::runOnModule(llvm::Module&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x28693b2) llvm#18 0x000055a3a35a499c llvm::legacy::PassManagerImpl::run(llvm::Module&) (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x286199c) llvm#19 0x000055a3a262abbb main (/home/davide/Ps2/rps2-tools/prefix/bin/llc+0x18e7bbb) llvm#20 0x00007f00bde3fc4c __libc_start_call_main ./csu/../sysdeps/nptl/libc_start_call_main.h:74:3 llvm#21 0x00007f00bde3fd05 call_init ./csu/../csu/libc-start.c:128:20 llvm#22 0x00007f00bde3fd05 __libc_start_main@GLIBC_2.2.5 ./csu/../csu/libc-start.c:347:5 llvm#23 0x000055a3a2624921 _start /builddir/glibc-2.39/csu/../sysdeps/x86_64/start.S:117:0 ``` This is caused by the fact that after the change, `f16`s are no longer lowered as `f32`s in calls. Two possible fixes are available: - Update calling conventions to properly support passing `f16` as integers. - Update `useFPRegsForHalfType()` to return `true` so that `f16` are still kept in `f32` registers, as before llvm#110199. This PR implements the first solution to not introduce any more ABI changes as llvm#110199 already did. As of what is the correct ABI for halfs, I don't think there is a correct answer. GCC doesn't support halfs on MIPS, and I couldn't find any information on old MIPS ABI manuals either.

yingopq force-pushed the Fix_bug_issue_97975 branch from 2ffa43e to 260ff02 Compare September 27, 2024 02:48

nikic reviewed Sep 27, 2024

View reviewed changes

yingopq force-pushed the Fix_bug_issue_97975 branch from 260ff02 to a618b49 Compare September 27, 2024 07:28

yingopq force-pushed the Fix_bug_issue_97975 branch from a618b49 to 38fcdfa Compare September 30, 2024 09:19

yingopq changed the title ~~[MIPS] Fix miscompiles consecutive half operations~~ [MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat Sep 30, 2024

nikic reviewed Oct 1, 2024

View reviewed changes

This was referenced Oct 1, 2024

LLVM miscompiles consecutive half operations by using too much precision on several backends #97975

Open

LLVM miscompiles passing/returning half on several backends by using lossy conversions #97981

Open

yingopq force-pushed the Fix_bug_issue_97975 branch from 38fcdfa to 3e5c16f Compare October 8, 2024 08:06

nikic pushed a commit that referenced this pull request Nov 5, 2024

[Mips] Add additional half float tests (NFC)

a256e89

For #110199.

[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteF…

a87fac2

…loat Fix part of llvm#97975.

nikic force-pushed the Fix_bug_issue_97975 branch from 3e5c16f to a87fac2 Compare November 5, 2024 09:18

nikic approved these changes Nov 5, 2024

View reviewed changes

nikic merged commit f0231b6 into llvm:main Nov 5, 2024
8 checks passed

PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this pull request Nov 6, 2024

[Mips] Add additional half float tests (NFC)

f9f4ca0

For llvm#110199.

PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this pull request Nov 6, 2024

[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteF…

bf06e05

…loat (llvm#110199) Fix part of llvm#97975.

kateinoigakukun mentioned this pull request Nov 12, 2024

Re-enable UseAggressiveReg2MemForCodeSize on Wasm swiftlang/swift#77567

Open

Tazdevil971 mentioned this pull request Nov 18, 2024

[MIPS] Updated MIPS N calling conventions so that fp16 arguments no longer cause a crash #116569

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat #110199

[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat #110199

yingopq commented Sep 27, 2024 •

edited

Loading

github-actions bot commented Sep 27, 2024 •

edited

Loading

nikic left a comment

nikic Sep 27, 2024

yingopq Sep 27, 2024

yingopq Sep 27, 2024

yingopq commented Sep 27, 2024

nikic Oct 1, 2024

yingopq Oct 8, 2024

yingopq Oct 18, 2024

yingopq commented Oct 30, 2024

tgross35 commented Nov 5, 2024 •

edited

Loading

nikic commented Nov 5, 2024

nikic left a comment

[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat #110199

[MIPS] Use softPromoteHalf legalization for fp16 rather than PromoteFloat #110199

Conversation

yingopq commented Sep 27, 2024 • edited Loading

github-actions bot commented Sep 27, 2024 • edited Loading

nikic left a comment

Choose a reason for hiding this comment

nikic Sep 27, 2024

Choose a reason for hiding this comment

yingopq Sep 27, 2024

Choose a reason for hiding this comment

yingopq Sep 27, 2024

Choose a reason for hiding this comment

yingopq commented Sep 27, 2024

nikic Oct 1, 2024

Choose a reason for hiding this comment

yingopq Oct 8, 2024

Choose a reason for hiding this comment

yingopq Oct 18, 2024

Choose a reason for hiding this comment

yingopq commented Oct 30, 2024

tgross35 commented Nov 5, 2024 • edited Loading

nikic commented Nov 5, 2024

nikic left a comment

Choose a reason for hiding this comment

yingopq commented Sep 27, 2024 •

edited

Loading

github-actions bot commented Sep 27, 2024 •

edited

Loading

tgross35 commented Nov 5, 2024 •

edited

Loading