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

Change scratch register of tail to t2 when Zicfilp enabled. #93

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

yetingk
Copy link
Contributor

@yetingk yetingk commented Jan 16, 2024

This change is to make tail conform with software guarded jump of Zicfilp. The reason to not choose t1 as the label register is that t1 is also as .got.plt offset of _dl_runtime_resolve in PLT.

riscv-asm.md Outdated Show resolved Hide resolved
@yetingk yetingk changed the title Change scratch register of tail to t2. Change scratch register of tail to t2 when Zicfilp enabled. Jan 16, 2024
Copy link
Contributor

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

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

OK with me, but I'd like to know if there actually is a problem with unconditionally changing to t2. Having both options seems like an unnecessary complication (although I concede it's only a very minor complication).

@kito-cheng
Copy link
Collaborator

cc @ved-rivos @jrtc27 @asb

@kito-cheng
Copy link
Collaborator

@aswaterman The reason I suggest keep both is I don't want make older tool become non-conformance with this asm manual, but maybe we could using different way/wording to prevent that?

@jrtc27
Copy link
Contributor

jrtc27 commented Jan 17, 2024

I still believe the extension should have been defined in a way that doesn't break existing practices, and it's pretty weird to have tail change which register it uses based on what extensions you happen to have enabled. That sounds like asking for trouble if people are assuming the register that tail clobbers (thinking about custom calling conventions when branching to a non-preemptible symbol), too, given it has been explicitly specified as being t1...

@jrtc27
Copy link
Contributor

jrtc27 commented Jan 17, 2024

Without thinking through all the implications, my instincts are that it is much better to change the PLT implementation (which is technically also specified today, but really shouldn't be, the code should just be an example of a possible implementation, as application code shouldn't know/care about it) than to change the semantics of pseudo-instructions.

Copy link
Contributor

@nick-knight nick-knight left a comment

Choose a reason for hiding this comment

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

Whatever change is made here should also be reflected in the table.

@ved-rivos
Copy link

Looks good to me.

@yetingk
Copy link
Contributor Author

yetingk commented Jan 19, 2024

Whatever change is made here should also be reflected in the table.

Done.

Copy link
Collaborator

@kito-cheng kito-cheng left a comment

Choose a reason for hiding this comment

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

Ideally we should keep those pseudo instructions consistency in every situation, but I know it's kinda trade off in ISA design, so LGTM from my side as well.

yetingk pushed a commit to yetingk/llvm-project that referenced this pull request Apr 17, 2024
PseudoTail should be a software guarded branch in Ziciflp, since its branch
target is known in link time. JALR/C.JR/C.JALR with rs1 as t2 is termed a
software guarded branch. Such branches do not need to land on a lpad instruction.

ABI Change PR: riscv-non-isa/riscv-asm-manual#93
riscv-asm.md Outdated Show resolved Hide resolved
riscv-asm.md Outdated Show resolved Hide resolved
yetingk added a commit to llvm/llvm-project that referenced this pull request May 2, 2024
PseudoTail should be a software guarded branch in Ziciflp, since its
branch target is known in link time. JALR/C.JR/C.JALR with rs1 as t2 is
termed a software guarded branch. Such branches do not need to land on a
lpad instruction.

ABI Change PR: riscv-non-isa/riscv-asm-manual#93
@kito-cheng
Copy link
Collaborator

NOTE: public review period for zicfiss and zicfilp are ended, waiting for ratification flow and then we will merge this.

@cmuellner
Copy link
Collaborator

Zicfiss, Zicfilp were ratified in June.

This change is to make `tail` conform with software guarded jump of [Zicfilp].
The reason to not choose `t1` as the label register is that `t1` is also as
`.got.plt` offset of `_dl_runtime_resolve` in [PLT].

[Zicfilp]: https://github.com/riscv/riscv-cfi/blob/main/cfi_forward.adoc
[PLT]: https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#procedure-linkage-table
@yetingk
Copy link
Contributor Author

yetingk commented Aug 2, 2024

Rebased to fix conflicts.

@kito-cheng kito-cheng merged commit f8bcdde into riscv-non-isa:main Aug 2, 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.

8 participants