Skip to content

Conversation

veselypeta
Copy link
Contributor

This PR:

  • Adds support the the RVY extension to be used alongside the existing xcheri instructions.
  • This change is based on the most up-to-date riscv-cheri specification
  • Changes are a cleaned up history based on codasip-cheri-riscv

I've done my best to create a sensible history, and can hopefully be reviewed by commit. There is still quite a large commit which adds most of the support for the new instructions and codegen (let me know if this shoud be divided up further).

@veselypeta veselypeta marked this pull request as ready for review August 20, 2025 15:00
@jrtc27
Copy link
Member

jrtc27 commented Aug 20, 2025

This is going to take a while to review, but two points I can make right now are that zRVY should be inferred from the arch string in .RISCV.attributes (and probably some riscvFoo member), and that we're not going to take the mapping symbols in that form (if you can infer the mode from the ISA string then you don't need to do anything new, and if we can't then $x should be something else like $y; Morello used $c but ARC has a vendetta against C). I'd suggest just leaving them out, they're not required to support RVY, it can be future work to make mixed-mode code disassemble better.

@jrtc27
Copy link
Member

jrtc27 commented Aug 20, 2025

Also it's [RISCV] for commit message prefixes, and RVY not rvy.

@jrtc27
Copy link
Member

jrtc27 commented Aug 20, 2025

[lld][RVY] Emit rvy instructions into .got.plt

.got.plt is the table of pointers, .plt is the instructions.

Copy link
Member

@arichardson arichardson left a comment

Choose a reason for hiding this comment

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

Thanks, this is great! Haven't got through the entire diff yet, but here is what I managed to review so far.

Comment on lines +16 to +18
// RUN: | FileCheck %s --check-prefixes=CHECK,RV32-ZYHYBRID '-DFEATURES=+32bit,+xcheri-norvc,+y,+zyhybrid'
// RUN: %clang --target=riscv64 -march=rv64izyhybrid -S -mno-xcheri-rvc -emit-llvm %s -o - \
// RUN: | FileCheck %s --check-prefixes=CHECK,RV64-ZYHYBRID '-DFEATURES=+64bit,+xcheri-norvc,+y,+zyhybrid'
Copy link
Member

Choose a reason for hiding this comment

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

I would drop all the +xcheri-norvc here. For RVY it should be based on the C extension.

Copy link
Member

Choose a reason for hiding this comment

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

@jrtc27 do we still need this flag?

Copy link
Member

Choose a reason for hiding this comment

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

I think I kept it so we could measure the gain from supporting compressing CHERI instructions, specifically thinking about numbers I might want for my PhD thesis. But it may not be quite right as it stands anyway (you probably want to compare with not remapping any of the C instructions, rather than disabling the ones that we remap), and can always be added back in a branch for that experiment.

Comment on lines +1 to +8
// RUN: %riscv64y_purecap_cc1 -E -dM -ffreestanding < /dev/null \
// RUN: | FileCheck %s
// RUN: %riscv32y_purecap_cc1 -E -dM -ffreestanding < /dev/null \
// RUN: | FileCheck %s
// RUN: %riscv64y_hybrid_cc1 -E -dM -ffreestanding < /dev/null \
// RUN: | FileCheck --check-prefixes=CHECK,CHECK-HYBRID %s
// RUN: %riscv32y_hybrid_cc1 -E -dM -ffreestanding < /dev/null \
// RUN: | FileCheck --check-prefixes=CHECK,CHECK-HYBRID %s
Copy link
Member

Choose a reason for hiding this comment

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

I would add a "--implicit-check-not=#define __CHERI" instead of the explicit CHECK-NOT lines.

}

MCSubtargetInfo &RISCVAsmPrinter::copySTI() {
RISCVSubtarget &STICopy = *static_cast<RISCVSubtarget *>(
Copy link
Member

Choose a reason for hiding this comment

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

Once we've made one copy, can't we just freely modify the feature bits on that one without having to create a new copy each time? So maybe just have a local pointer that is null initially and gets set on the first copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for having to copy is that RISCVAsmPrinter stores a pointer to const of the subtarget info, so we copy-modify-replace to maintain the 'constness'. This is also done like this in RISCVAsmParser

@veselypeta veselypeta force-pushed the petr/implement-rvy branch 3 times, most recently from 3bc2132 to 4ba003b Compare August 26, 2025 11:56
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