Skip to content

Conversation

@keith-packard
Copy link

Build a soft float multilib variant for aarch64 supporting targets without an FPU.

This contains a couple of minor fixes for llvm to enable this; I'll be submitting those upstream.

Copy link
Collaborator

@ostannard ostannard left a comment

Choose a reason for hiding this comment

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

For the LLVM changes, we're trying to minimise the side of our downstream patch, so we'd prefer to wait until these have been committed to LLVM itself.

For the testing, have you checked if QEMU correctly models the undefined instruction exception if a floating-point instruction is executed in this library variant? I did some testing a while ago and couldn't get that to happen, which is why I'm trying to make FVPs usable for testing, since they can accurately model a lot of less-common architectures like this.

- ldp x10,x11, [x0, #0x050]
- ldp x12,x13, [x0, #0x060]
- ldp x14,x15, [x0, #0x070]
+ LDP(x2, x3, x0, #0x010, #0x018)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The LDP instructions with X registers should be available without the FPU, is this part of the change needed?

Copy link
Author

Choose a reason for hiding this comment

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

Nope. I'm not sure why I thought that was necessary. I'll be updating the upstream PR once I've finish testing.

Build a soft float multilib variant for aarch64 supporting targets
without an FPU.

This contains a couple of minor fixes for llvm to enable this; I'll be
submitting those upstream.

Signed-off-by: Keith Packard <[email protected]>
@keith-packard
Copy link
Author

For the LLVM changes, we're trying to minimise the side of our downstream patch, so we'd prefer to wait until these have been committed to LLVM itself.

Makes sense. I'm working on getting those upstream.

For the testing, have you checked if QEMU correctly models the undefined instruction exception if a floating-point instruction is executed in this library variant? I did some testing a while ago and couldn't get that to happen, which is why I'm trying to make FVPs usable for testing, since they can accurately model a lot of less-common architectures like this.

I don't think QEMU has a model for aarch64 without an FPU, so all of my testing uses the full machine. However, my test harness leaves the FPU disabled when testing the nofp code. When it hits an instruction using the FPU, qemu traps.

@keith-packard
Copy link
Author

It would be good to have these changes tested on FVP; I haven't figured out how to do that yet.

@pratlucas
Copy link
Collaborator

Hi @keith-packard ,
Now that llvm/llvm-project#111235 has been merged, I believe this change can go ahead.
There were some significant changes to the cmake setup of the project recently (from #562), so it might require a significant update to the code for the new variant. Would you like me to handle the rebase on a separate PR for you (crediting the changes to you as co-author)? Or would you prefer to do it yourself?

@keith-packard
Copy link
Author

Hi @keith-packard , Now that llvm/llvm-project#111235 has been merged, I believe this change can go ahead. There were some significant changes to the cmake setup of the project recently (from #562), so it might require a significant update to the code for the new variant. Would you like me to handle the rebase on a separate PR for you (crediting the changes to you as co-author)? Or would you prefer to do it yourself?

I'm happy to let you get it fixed up and ready to merge.

@vhscampos
Copy link
Member

I've opened #588 after a rebase on top of the current latest.

@voltur01
Copy link
Contributor

#588 merged, so I am closing this one.

@voltur01 voltur01 closed this Dec 20, 2024
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.

5 participants