This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][RISCV] Support reading of VLENB CSR register
ClosedPublic

Authored by kachkov98 on Oct 19 2022, 8:59 AM.

Details

Summary

Support read of VLENB (vector byte length) control register (CSR number: 0xC22, DWARF register number: 0x1C22 according to RISC-V DWARF specification: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-dwarf.adoc). This support is needed for correct unwinding of RVV objects on stack.

Required for fix of https://github.com/llvm/llvm-project/issues/58356

Diff Detail

Event Timeline

kachkov98 created this revision.Oct 19 2022, 8:59 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 19 2022, 8:59 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
kachkov98 requested review of this revision.Oct 19 2022, 8:59 AM
kito-cheng accepted this revision.Oct 20 2022, 9:32 PM

LGTM, just one nit :)

libunwind/src/Registers.hpp
4106

nit: __asm__("csrr %0, 0xC22" : "=r"(vlenb));

kachkov98 updated this revision to Diff 469504.Oct 21 2022, 1:44 AM

Review changes

kito-cheng accepted this revision.Oct 21 2022, 1:46 AM

Still LGTM, I guess we need some libunwind guy to approve that?

MaskRay requested changes to this revision.EditedOct 24 2022, 7:42 PM

Can you write a test? Run tests with ninja check-unwind

Changing _LIBUNWIND_HIGHEST_DWARF_REGISTER to such a large integer makes
RegisterLocation savedRegisters[kMaxRegisterNumber + 1]; consume a lot of stack space and harms performance.

libunwind/include/libunwind.h
1026
This revision now requires changes to proceed.Oct 24 2022, 7:42 PM

Please mention in the summary the official document defining UNW_RISCV_VLENB. Why is it so large?

kachkov98 updated this revision to Diff 470493.Oct 25 2022, 7:47 AM

Add test and review fixes

Please mention in the summary the official document defining UNW_RISCV_VLENB. Why is it so large?

Link to the documentation: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-dwarf.adoc - it's already placed before RISC-V registers enum, I've also added it to the commit message and copied DWARF numbers from the table to the enum comment

Can you write a test? Run tests with ninja check-unwind

Changing _LIBUNWIND_HIGHEST_DWARF_REGISTER to such a large integer makes
RegisterLocation savedRegisters[kMaxRegisterNumber + 1]; consume a lot of stack space and harms performance.

Thank you, I didn't noticed that. Fortunately, this register is read-only and we don't need to save it, so in this particular case it looks ok to not change MaxRegisterNumber at all. Looks like NEC's Vector Engine target in libunwind already does the same thing for the UNW_VE_VIXR, UNW_VE_VL registers.

kachkov98 edited the summary of this revision. (Show Details)Oct 25 2022, 7:58 AM
MaskRay requested changes to this revision.Nov 19 2022, 2:25 PM

Support read of VLENB (vector byte length) control register (CSR number: 0xC22, DWARF register number: 0x1C22 according to RISC-V DWARF specification: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-dwarf.adoc). This support is needed for correct unwinding of RVV objects on stack.

I don't find 0x1C22 on https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-dwarf.adoc

No, please don't share false information.

This revision now requires changes to proceed.Nov 19 2022, 2:25 PM

Support read of VLENB (vector byte length) control register (CSR number: 0xC22, DWARF register number: 0x1C22 according to RISC-V DWARF specification: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-dwarf.adoc). This support is needed for correct unwinding of RVV objects on stack.

I don't find 0x1C22 on https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-dwarf.adoc

No, please don't share false information.

The DWARF spec generically says CSRs are assigned their CSR address plus 4096.

Support read of VLENB (vector byte length) control register (CSR number: 0xC22, DWARF register number: 0x1C22 according to RISC-V DWARF specification: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-dwarf.adoc). This support is needed for correct unwinding of RVV objects on stack.

I don't find 0x1C22 on https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-dwarf.adoc

No, please don't share false information.

The DWARF spec generically says CSRs are assigned their CSR address plus 4096.

This information helps but is still unsatisfactory.

The RISC-V specification defines a total of 4096 CSRs (see <<riscv-priv>>).
Each CSR is assigned a DWARF register number corresponding to its specified CSR
number plus 4096.

[bibliography]
== References

* [[[riscv-priv]]] "The RISC-V Instruction Set Manual, Volume II: Privileged
Architecture, Document", Editors Andrew Waterman, Krste Asanovi´c, and
John Hauser, RISC-V International.

I don't think [[riscv-priv] assigns 0xc22 to VLENB. Referencing v-spec.adoc in riscv-elf-psabi-doc will probably help.

kachkov98 updated this revision to Diff 476823.Nov 21 2022, 1:08 AM

Added link to v-spec with VLENB definition

MaskRay added inline comments.Dec 2 2022, 8:10 PM
libunwind/test/unwind_scalable_vectors.pass.cpp
16

static

16

Add a comment what this function does.

kachkov98 updated this revision to Diff 480088.Dec 5 2022, 6:25 AM

Addressing review comments

MaskRay added inline comments.Dec 5 2022, 12:28 PM
libunwind/test/unwind_scalable_vectors.pass.cpp
33

This is: DW_OP_breg 0 DW_OP_bregx 0x3822 0 DW_OP_plus.

Why is 0x3822 used?

MaskRay accepted this revision.Dec 5 2022, 12:29 PM

Looks great!

libunwind/test/unwind_scalable_vectors.pass.cpp
33

Ah, sorry, it's DW_OP_breg2 0 DW_OP_bregx 0x1C22 0 DW_OP_plus. This is correct.

This revision is now accepted and ready to land.Dec 5 2022, 12:29 PM
MaskRay added inline comments.Dec 5 2022, 12:30 PM
libunwind/test/unwind_scalable_vectors.pass.cpp
27

Since we use // REQUIRES: linux && target={{riscv64-.+}}, the #if is unneeded.

This revision was landed with ongoing or failed builds.Dec 6 2022, 12:50 AM
This revision was automatically updated to reflect the committed changes.
whitequark added a subscriber: whitequark.EditedJul 30 2023, 8:59 PM

I think this revision introduces a bug: namely, without Zicsr enabled, the code would not compile.

The code in getRegister should be wrapped in #if defined(__riscv_v) and if it's not enabled then 0 would be returned.

evandro removed a subscriber: evandro.Aug 2 2023, 12:59 PM