Skip to content

[RISCV] Add thread-pointer relaxations for local-exec TLS model#1534

Open
Jonathon Penix (jonathonpenix) wants to merge 3 commits into
qualcomm:mainfrom
jonathonpenix:pr/riscv_tls
Open

[RISCV] Add thread-pointer relaxations for local-exec TLS model#1534
Jonathon Penix (jonathonpenix) wants to merge 3 commits into
qualcomm:mainfrom
jonathonpenix:pr/riscv_tls

Conversation

@jonathonpenix

Copy link
Copy Markdown
Contributor

Implemented as defined in the psABI at [1]

Broadly, local-exec TLS accesses may be relaxed from the full sequence (ex: lui/add/lw) to a single tp-relative instruction (ex: a tp-relative lw) if the offset is within +-2KB. We check for offset < 2048 as I don't think a negative tp offset is meaningful in "normal" operation.

While here, I'm also removing the usage of applyTprelAdd--as far as I'm aware there are no other psABI-defined relaxations around this relocation, so this should be obsolete.

A few misc. items to note:

  • This can misrelax the sequence if part is marked relaxable and part is not. I'm not sure there's a great way to prevent this that works in general and this is a) in line with some other RISC-V relaxations in eld and b) is consistent with what lld and bfd do. So I didn't worry about trying to handle this.
  • You can also see weird things happen if you write (probably erroneous) assembly with different addends. As far as I can tell, eld is relaxing such sequences consistently with lld/bfd, so I left it be.
  • I think it should be possible to change the tp-relative offset after the relaxation happens, potentially causing a mislink. And I don't think we have any error checking that would catch these cases currently. This should be consistent with ex: the TLSDESC short-form relaxations, so I'm hoping to address this as a follow-up patch.

[1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/e03d44ae2f0e1144f9498c2896b5ae25b0449398/riscv-elf.adoc?plain=1#L2531

Implemented as defined in the psABI at [1]

Broadly, local-exec TLS accesses may be relaxed from the full sequence (ex:
lui/add/lw) to a single tp-relative instruction (ex: a tp-relative lw) if the
offset is within +-2KB. We check for offset < 2048 as I don't think a negative
tp offset is meaningful in "normal" operation.

While here, I'm also removing the usage of applyTprelAdd--as far as I'm aware
there are no other psABI-defined relaxations around this relocation, so this
should be obsolete.

A few misc. items to note:
* This can misrelax the sequence if part is marked relaxable and part is not.
  I'm not sure there's a great way to prevent this that works in general and
  this is a) in line with some other RISC-V relaxations in eld and b) is
  consistent with what lld and bfd do. So I didn't worry about trying to
  handle this.
* You can also see weird things happen if you write (probably erroneous)
  assembly with different addends. As far as I can tell, eld is relaxing such
  sequences consistently with lld/bfd, so I left it be.
* I *think* it should be possible to change the tp-relative offset *after* the
  relaxation happens, causing a mislink. And I don't think we have any error
  checking that would catch these cases currently. This should be consistent
  with ex: the TLSDESC short-form relaxations, so I'm hoping to address this
  as a follow-up patch.

[1] https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/e03d44ae2f0e1144f9498c2896b5ae25b0449398/riscv-elf.adoc?plain=1#L2531

Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>
Signed-off-by: Jonathon Penix <jpenix@qti.qualcomm.com>

@quic-areg quic-areg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@jonathonpenix

Copy link
Copy Markdown
Contributor Author

Just linking this comment/request for changes from Shankar: #1428 (comment)

Will come back to/update this accordingly

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.

2 participants