Skip to content
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

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

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

Tazdevil971
Copy link
Contributor

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, f16s are no longer lowered as f32s in calls.

Two possible fixes are available:

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.

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. In the absence of a specified ABI and hardware support, I believe passing in integers is preferable.

cc @yingopq

@Tazdevil971
Copy link
Contributor Author

Thanks! I currently do not have write permissions on this repo, so I cannot merge it myself.

@yingopq
Copy link
Contributor

yingopq commented Nov 19, 2024

I did not understand why the order need to be reversed. Thanks!

@Tazdevil971
Copy link
Contributor Author

So, the current implementation does the following:

  • Convert i8/i16/i32 to i64 only if they were not originally floats
  • Handle any remaining i32, only on soft-float, in a special way (these i32s are actually floats in the original signature)

But after the change, f16 were turned into i16, so they cannot handled by the first case because the i16 was originally a float, but also they cannot be handled by the second case because they aren't i32s, causing the crash.

What I did was relax the conditions, so that i16 that were originally float could still be handled by the first case, and the simplest and clearest way was to flip the order and make the second case more specific.

So now the logic is as follows:

  • Handle any i32, on soft-float only and if they were originally float (this should now cover the second case)
  • Convert i8/i16/i32 to i64 (any i32 that needed to be handled in a special case would have been processed already)

In order to not flip the order of these two operations we would need to make the first condition more complicated than "they were not originally floats".

@yingopq
Copy link
Contributor

yingopq commented Nov 19, 2024

So, the current implementation does the following:

  • Convert i8/i16/i32 to i64 only if they were not originally floats
  • Handle any remaining i32, only on soft-float, in a special way (these i32s are actually floats in the original signature)

But after the change, f16 were turned into i16, so they cannot handled by the first case because the i16 was originally a float, but also they cannot be handled by the second case because they aren't i32s, causing the crash.

What I did was relax the conditions, so that i16 that were originally float could still be handled by the first case, and the simplest and clearest way was to flip the order and make the second case more specific.

So now the logic is as follows:

  • Handle any i32, on soft-float only and if they were originally float (this should now cover the second case)
  • Convert i8/i16/i32 to i64 (any i32 that needed to be handled in a special case would have been processed already)

In order to not flip the order of these two operations we would need to make the first condition more complicated than "they were not originally floats".

Thank you for your detailed reply, I got it.
LGTM.

@nikic nikic merged commit 8cd348c into llvm:main Nov 19, 2024
9 checks passed
Copy link

@Tazdevil971 Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

swatheesh-mcw pushed a commit to swatheesh-mcw/llvm-project that referenced this pull request Nov 20, 2024
…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.
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.

3 participants