-
Notifications
You must be signed in to change notification settings - Fork 179
Fix TLSDESC description #420
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
Conversation
The table specifies R_RISCV_TLSDESC_LOAD_LO12 and R_RISCV_TLSDESC_ADD_LO12, while some text uses the `_I` suffix. Remove the `_I` suffix to be compatible with the proposed binutils patch. `auipc a0` in a relaxation result should be `auipc tX`.
|
The _I change looks good to me. I’m not sure if the tX change is necessary. Both a0 and tX are legal choices here (although tY is not guaranteed to be alive because LOAD is allowed to be reordered after ADD). I think a0 is better overall because the resulting instruction sequence is macro-fusable if both instructions are adjacent. |
|
I think it makes sense that things are kept consistent with the initial template, so the use of If there's a big concern about that, then maybe we can add aa comment/note that using |
I’m not sure if I see why this is more consistent. The destination register for the ADD instruction is a0, so I think it makes more sense for the relaxed instruction to target a0 too. It’s fine to clarify that both a0/tX can be used, but if we want to pick one, I don’t see any reason so far why tX should be used instead. |
I was thinking whether we needed the flexibility but I guess the answer is probably no. |
ishitatsuyuki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Fix comment typos in #66915, and relocation type names related to the example in the psABI (riscv-non-isa/riscv-elf-psabi-doc#420).
kito-cheng
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :)
The table specifies R_RISCV_TLSDESC_LOAD_LO12 and
R_RISCV_TLSDESC_ADD_LO12, while some text uses the
_Isuffix. Remove the_Isuffix to be compatible withLLVM and the proposed binutils patch.