7
31
Fork
You've already forked blackmagic
23

riscv : add basic single precision hw float support #1766

Merged
mean00 merged 1 commit from riscv_single_fp_support into main 2024年02月18日 12:24:09 +01:00
mean00 commented 2024年02月15日 08:01:57 +01:00 (Migrated from github.com)
Copy link

Detailed description

  • This adds basic support for hw single precision fpu (rv32)

  • Without the patch gdb will reject the binary as the register list is incomplete

  • This is not exactly the same as the patch posted in the bug report, it actually connects the registers

  • With that patch on a CH32V307 , i can load the binary and print the registers with a value that looks correct (limited testing), seems ok for fs0, f1, fs2 , fs3

  • Tested only on rv32

  • May lack some CSR relatated to FPU declared as CSR , not clear.

  • I've read the Code of Conduct

  • I've read the guidelines for contributing to this repository

  • [up to linking then out of rom ] It builds for hardware native (see Building the firmware)

  • It builds as BMDA (see Building the BMDA)

  • I've tested it to the best of my ability

  • My commit messages provide a useful short description of what the commits do

Closing issues

fixes #1765

## Detailed description * This adds basic support for hw single precision fpu (rv32) * Without the patch gdb will reject the binary as the register list is incomplete * This is not exactly the same as the patch posted in the bug report, it actually connects the registers * With that patch on a CH32V307 , i can load the binary and print the registers with a value that looks correct (limited testing), seems ok for fs0, f1, fs2 , fs3 * Tested only on rv32 * May lack some CSR relatated to FPU declared as CSR , not clear. * [X] I've read the [Code of Conduct](https://github.com/blackmagic-debug/blackmagic/blob/main/CODE_OF_CONDUCT.md) * [X] I've read the [guidelines for contributing](https://github.com/blackmagic-debug/blackmagic/blob/main/CONTRIBUTING.md) to this repository * [up to linking then out of rom ] It builds for hardware native (see [Building the firmware](https://github.com/blackmagic-debug/blackmagic?tab=readme-ov-file#building-black-magic-debug-firmware)) * [X] It builds as BMDA (see [Building the BMDA](https://github.com/blackmagic-debug/blackmagic?tab=readme-ov-file#building-black-magic-debug-app)) * [X] I've tested it to the best of my ability * [X] My commit messages provide a useful short description of what the commits do ## Closing issues fixes #1765
dragonmux left a comment
Copy link

This is looking good though needs clang-format run - please see our review notes below for the stylistic items we've spotted and some code organisation suggestions.

With those taken care of, this should be good to merge.

This is looking good though needs `clang-format` run - please see our review notes below for the stylistic items we've spotted and some code organisation suggestions. With those taken care of, this should be good to merge.
@ -153,6 +153,8 @@ static ssize_t riscv32_reg_read(target_s *target, const uint32_t reg, void *data
return riscv32_bool_to_4(riscv_csr_read(hart, RV_DPC, data));

Please drop the additional braces in this and the register write if statement additions - they're not necessary. To be more consistent with the style of the existing code and how things are done in the other architecture implementations, may we suggest RV_FPU_GDB_OFFSET be used for this constant?

Please also run clang-format - there's an extra space in the second change here, and this + the other added if statement are missing one between if and the ( for the condition.

Please drop the additional braces in this and the register write if statement additions - they're not necessary. To be more consistent with the style of the existing code and how things are done in the other architecture implementations, may we suggest `RV_FPU_GDB_OFFSET` be used for this constant? Please also run clang-format - there's an extra space in the second change here, and this + the other added if statement are missing one between `if` and the `(` for the condition.
@ -159,0 +154,4 @@
{"mtvec", RV_CSR_MTVEC},
{"tscratch", RV_CSR_MSCRATCH},
{"mepc", RV_CSR_MEPC},
{"mcause", RV_CSR_MCAUSE},

The final string needs to be followed by a , to get clang-format to format this properly.

The final string needs to be followed by a `,` to get clang-format to format this properly.

One notable thing from our prior work with GDB here, we can likely drop both the group= and type= stanzas which reduces the size of the feature blob generated here, which can otherwise get rather large. This reduces the likelihood of an OOM condition occurring on STM32F1-based probes.

One notable thing from our prior work with GDB here, we can likely drop both the `group=` and `type=` stanzas which reduces the size of the feature blob generated here, which can otherwise get rather large. This reduces the likelihood of an OOM condition occurring on STM32F1-based probes.

Please use the ARRAY_LENGTH() macro rather than computing the array length yourself here. Likewise size_t not unsigned int - the type of sizeof() is always size_t, not using this type alias may lead to type conversions.

Please use the `ARRAY_LENGTH()` macro rather than computing the array length yourself here. Likewise `size_t` not `unsigned int` - the type of sizeof() is always size_t, not using this type alias may lead to type conversions.

Given this needs to essentially be done again for doubles, please move this into its own function above this one. That way it can be re-used more easily to build out the RV_ISA_EXT_DOUBLE_FLOAT handling.

Given this needs to essentially be done again for doubles, please move this into its own function above this one. That way it can be re-used more easily to build out the `RV_ISA_EXT_DOUBLE_FLOAT` handling.

The original idea with this section of defines is that it was a block just for the CSRs - perhaps move these two new defines below to line 218 as a new block just for FPU definitions?

The original idea with this section of defines is that it was a block just for the CSRs - perhaps move these two new defines below to line 218 as a new block just for FPU definitions?
mean00 commented 2024年02月17日 08:29:29 +01:00 (Migrated from github.com)
Copy link

Thank you
I've fixed most if not all as a separate commit. If it's okay i'll squash them

Thank you I've fixed most if not all as a separate commit. If it's okay i'll squash them

It's looking much better. Please make sure you run clang-format on the changes as there are still inconsistencies that it will take care of, and that leaves just one review item to discuss and address.

It's looking much better. Please make sure you run `clang-format` on the changes as there are still inconsistencies that it will take care of, and that leaves just one review item to discuss and address.
mean00 commented 2024年02月17日 11:15:18 +01:00 (Migrated from github.com)
Copy link

I have both git pre commit installed & manually ran clang-format before commiting (twice).
I'm beginning to suspect having clang-fmt 17 is causing the issues. It looks like bmp is setup for clang-fmt16. Maybe a known gotcha ?

Regarding extracting the function, i would have to forward fpu size (32/64) but also offset, buffer, .... as parameters
Am i missing something or is it the idea ?

Thank you

I have both git pre commit installed & manually ran clang-format before commiting (twice). I'm beginning to suspect having clang-fmt 17 is causing the issues. It looks like bmp is setup for clang-fmt16. Maybe a known gotcha ? Regarding extracting the function, i would have to forward fpu size (32/64) but also offset, buffer, .... as parameters Am i missing something or is it the idea ? Thank you

clang-format-17 should work fine (it's clang-format-18 that does extra stuff that's unwanted here), but failing that use clang-format-16 via pip (https://github.com/ssciwr/clang-format-wheel)

Re extracting the function: yes, that's exactly the idea - see github.com/blackmagic-debug/blackmagic@c4015cdf5e/src/target/cortexar.c (L1682-L1704) and the Cortex-M equivalent which explicitly and intentionally splits out FPU block generation to manage routine complexity and size. Ideally the CSR block would likewise be split out.

clang-format-17 should work fine (it's clang-format-18 that does extra stuff that's unwanted here), but failing that use clang-format-16 via `pip` (https://github.com/ssciwr/clang-format-wheel) Re extracting the function: yes, that's exactly the idea - see https://github.com/blackmagic-debug/blackmagic/blob/c4015cdf5efaeaca35075f04f461d72132aa3a37/src/target/cortexar.c#L1682-L1704 and the Cortex-M equivalent which explicitly and intentionally splits out FPU block generation to manage routine complexity and size. Ideally the CSR block would likewise be split out.
mean00 commented 2024年02月17日 17:09:38 +01:00 (Migrated from github.com)
Copy link

Thank you for the details
Hopefully this time it is ok.
I've moved the fpu part to its own function (like for CM-AR) and added a fpu size parameter so it should be easier to add support for double precision later (dont have any of those available to test).

(triple checked clang-format (17) was not complaining, not too hopeful)

Thank you for the details Hopefully this time it is ok. I've moved the fpu part to its own function (like for CM-AR) and added a fpu size parameter so it should be easier to add support for double precision later (dont have any of those available to test). (triple checked clang-format (17) was not complaining, not too hopeful)
dragonmux left a comment
Copy link

Think we've finally figured out why clang-format was not formatting those FPU regs strings - see our review notes. This is almost there. There's a query we've got on two lines in the FPU block generator and a couple of suggestions and once those are answered and taken care of w/ the one necessary fix made, this should be good to merge.

Think we've finally figured out why clang-format was not formatting those FPU regs strings - see our review notes. This is almost there. There's a query we've got on two lines in the FPU block generator and a couple of suggestions and once those are answered and taken care of w/ the one necessary fix made, this should be good to merge.
@ -159,0 +154,4 @@
{"mtvec", RV_CSR_MTVEC},
{"tscratch", RV_CSR_MSCRATCH},
{"mepc", RV_CSR_MEPC},
{"mcause", RV_CSR_MCAUSE},

This needs to go before your addition so that clang-format does its thing on it. This is why things aren't getting formatted appropriately.

This needs to go before your addition so that clang-format does its thing on it. This is why things aren't getting formatted appropriately.

Given the order of parameters on the base function, the compiler will likely generate a smaller amount of code for the call frame if fpu_size is made the last parameter. This is fine otherwise.

Given the order of parameters on the base function, the compiler will likely generate a smaller amount of code for the call frame if `fpu_size` is made the last parameter. This is fine otherwise.

It would be useful with this and the line above if you could expand on what needs fixing about them - it's very curious what those "fixme!" comments are about.

It would be useful with this and the line above if you could expand on what needs fixing about them - it's very curious what those "fixme!" comments are about.
@ -921,0 +1001,4 @@
* <reg name="fflags" bitsize="32" regnum="66" save-restore="no"/>
* <reg name="frm" bitsize="32" regnum="67" save-restore="no"/>
* <reg name="fcsr" bitsize="32" regnum="68" save-restore="no"/>
*</feature>

We would move this comment block to the front of the FPU block function so the documentation for it is kept together with the function.

We would move this comment block to the front of the FPU block function so the documentation for it is kept together with the function.
mean00 (Migrated from github.com) reviewed 2024年02月18日 11:14:43 +01:00
mean00 (Migrated from github.com) commented 2024年02月18日 11:14:43 +01:00
Copy link

No sure i got it correctly, i think the idea was to move the clang-format on up

  • fixed a mistake on fpu csr param that were swapped
    Thank you
No sure i got it correctly, i think the idea was to move the clang-format on up + fixed a mistake on fpu csr param that were swapped Thank you

Think you'd meant to reply to the first of these 4 review items. Yes, that is the change we were thinking though you've moved it further up than we were anticipating - but think that's ok as that other code needed some formatting love too.

The only remaining issue there is the lack of a blank line between the preceding array and your addition, but as the CSR stuff needs some work, don't worry about that. We can fix that after.

Think you'd meant to reply to the first of these 4 review items. Yes, that is the change we were thinking though you've moved it further up than we were anticipating - but think that's ok as that other code needed some formatting love too. The only remaining issue there is the lack of a blank line between the preceding array and your addition, but as the CSR stuff needs some work, don't worry about that. We can fix that after.
dragonmux left a comment
Copy link

This LGTM now, we'll get this merged after it's commits have been squashed to remove fixups as we have no reasonable way to test it locally.

This LGTM now, we'll get this merged after it's commits have been squashed to remove fixups as we have no reasonable way to test it locally.
mean00 commented 2024年02月18日 11:54:45 +01:00 (Migrated from github.com)
Copy link

Squashed, thank you

Squashed, thank you
Sign in to join this conversation.
No reviewers
Labels
Clear labels
BMD App
Black Magic Debug App (aka. PC hosted) (not firmware)
BMP Firmware
Black Magic Probe Firmware (not PC hosted software)
Bug
Confirmed bug
Build system
Build system
Can't reproduce
Maintainers can't reproduce this problem
CI
Continuous Integration System
Contribution wanted
User contributions welcome
Documentation
Project documentation
Draft
Work in progress draft
Duplicate
This issue or pull request already exists
Enhancement
General project improvement
Feedback wanted
Requires additional submitter feedback
Foreign Host Board
Non Native hardware to runing Black Magic firmware on
GDB
Issue/PR related to GDB
Good first issue
Good for newcommers
HwIssue Mitigation
Solving or mitigating a Hardware issue in Software
Information Needed
Maintainers need more information
NativeHardware
Official Black Magic Debug Hardware
New Host Board
New hardware to run Black Magic firmware on
New Target
New debug target
Off Topic
Something that does not involve the project in any way
Potential Bug
A potential, unconfirmed or very special circumstance bug
Regression
Bug caused by a regression
User Interest Needed
More user interest required before consideration
User Testing Needed
Looking for user testing reports
Won't fix
Outside of the project scope or works as intended
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
blackmagic-debug/blackmagic!1766
Reference in a new issue
blackmagic-debug/blackmagic
No description provided.
Delete branch "riscv_single_fp_support"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?