Page MenuHomePhabricator

[RISCV] Add lowering of global TLS addresses
ClosedPublic

Authored by lewis-revill on Dec 4 2018, 5:19 PM.

Details

Summary

This patch adds lowering for global TLS addresses for the TLS models of InitialExec, GlobalDynamic, LocalExec and LocalDynamic.

LocalExec support required using a 4-operand add instruction, which uses the fourth operand to express a relocation on the symbol. The necessary fixup is emitted when the instruction is emitted.

Diff Detail

Repository
rL LLVM

Event Timeline

lewis-revill created this revision.Dec 4 2018, 5:19 PM
jrtc27 requested changes to this revision.Dec 4 2018, 7:11 PM

I'd rather see all the local-exec support moved into D55306; I don't think it makes much sense to add things here that don't yet work. Alternatively, to avoid lines that are then going to be modified again in D55306, you could merge the two? There aren't many extra changes in D55306.

This revision now requires changes to proceed.Dec 4 2018, 7:11 PM
asb added a subscriber: luismarques.Dec 5 2018, 2:13 AM

Thanks for sharing this patch series Lewis. As it happens @luismarques has been finishing off his own implementation of TLS support which this unfortunately clashes with. The upside is that we're in a good position to review this work!

Could you please refactor the patches so that MC layer changes occur separately to codegen. e.g. having one patch that adds the MC layer changes for for TLS and a second which adds the codegen changes.

I'd rather see all the local-exec support moved into D55306; I don't think it makes much sense to add things here that don't yet work. Alternatively, to avoid lines that are then going to be modified again in D55306, you could merge the two? There aren't many extra changes in D55306.

Yes sorry about that, I can do that.

In D55305#1319850, @asb wrote:

Thanks for sharing this patch series Lewis. As it happens @luismarques has been finishing off his own implementation of TLS support which this unfortunately clashes with. The upside is that we're in a good position to review this work!

Could you please refactor the patches so that MC layer changes occur separately to codegen. e.g. having one patch that adds the MC layer changes for for TLS and a second which adds the codegen changes.

OK, I'll look at doing that and incorporating existing MC patches where they exist.

lewis-revill planned changes to this revision.Dec 5 2018, 10:12 AM

Merging D55306 into this, then splitting this patch into MC layer and Codegen

lewis-revill retitled this revision from [RISCV, WIP] Add initial lowering of global TLS addresses to [RISCV, WIP] Add lowering of global TLS addresses.
lewis-revill edited the summary of this revision. (Show Details)

Merged D55306 into this patch.

lewis-revill edited the summary of this revision. (Show Details)

Split into MC layer (patches D55341 D55342) and codegen (this patch)

Rebased with updated dependencies.

lewis-revill edited the summary of this revision. (Show Details)

Rebase with updated dependencies. Use and expand the la.tls.ie and la.tls.gd pseudo instructions.

lewis-revill retitled this revision from [RISCV, WIP] Add lowering of global TLS addresses to [RISCV] Add lowering of global TLS addresses.

Rebased and fixed FileCheck lines.

lewis-revill added inline comments.May 20 2019, 5:47 AM
lib/Target/RISCV/RISCVISelLowering.cpp
468 ↗(On Diff #180636)

Am I correct in thinking this sequence should actually be opaque here, it causes a DAG with multiple load/stores of the same GlobalTLSAddress to only duplicate the ADDI relative to a single AddTPRel node... I know this would be safe for absolute addressing, but not for thread-pointer relative addressing?

Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 5:47 AM
Herald added subscribers: benna, psnobl. · View Herald Transcript
lewis-revill added inline comments.May 20 2019, 5:56 AM
lib/Target/RISCV/RISCVISelLowering.cpp
468 ↗(On Diff #180636)

Actually it appears GCC will do common subexpression elimination on this sequence with optimizations enabled so I would take that to mean it is safe to emit as it is.

rogfer01 added inline comments.May 21 2019, 12:39 AM
lib/Target/RISCV/RISCVISelLowering.cpp
445 ↗(On Diff #180636)

Because we run BranchRelaxation in addPreEmitPass but the expansion of these pseudos happens in addPreEmitPass2 I think we need to update RISCVInstrInfo::getInstSizeInBytes to state that PseudoLA_TLS_IE and PseudoLA_TLS_GD will take 8 bytes (otherwise we run into "fixup errors" because the relaxation has been done using an underestimation).

(Alternatively we could use the Size attribute in tablegen but currently only PseudoLI does this and we don't use that one in codegen, so that is probably OK)

Also I wonder if the attribute isAsmParserOnly = 1 should be updated to 0 so we are more honest with the current usage of these pseudos. That said from a cursory check in tablegen, that attribute does not seem very important at the moment.

lewis-revill added inline comments.May 21 2019, 1:31 AM
lib/Target/RISCV/RISCVISelLowering.cpp
445 ↗(On Diff #180636)

Thanks, I'll make that change, I did the same for the PIC PseudoLA so I must have forgotten it for this.

The isAsmParser attribute seems to enforce that the pseudo instruction doesn't cross the barrier between CodeGen and MC. Might be worth removing it to prevent confusion, I could do that in a later patch since there are now a couple of pseudos like this.

Add pseudo instructions to getInstSizeInBytes.

rogfer01 added inline comments.May 23 2019, 12:35 AM
lib/Target/RISCV/RISCVISelLowering.cpp
523 ↗(On Diff #200452)

I'm curious, is there a reason to have this unused parameter here? (other than symmetry with getStaticTLSAddr, that is).

Removing it could allow you to combine the two cases in lowerGlobalTLSAddress. That could make more obvious that in RISC-V we use the same sequence in both cases.

Added nounwind to test functions.

asb accepted this revision.Jun 18 2019, 3:45 AM

I added some very minor nits in comments, but other than those I think this is looking good to land. Thanks! It's also worth checking @rogfer01's suggestion before committing https://reviews.llvm.org/D55305#inline-553463

lib/Target/RISCV/RISCVISelLowering.cpp
500 ↗(On Diff #205054)

Line is longer than 80 chars, git clang-format will reformat niclely (assuming you have the git-clang-format helper on your PATH)

538 ↗(On Diff #205054)

Line is longer than 80 chars, git clang-format will reformat niclely (assuming you have the git-clang-format helper on your PATH)

574 ↗(On Diff #205054)

The enumeration is fully covered, so we shouldn't have the default case here: https://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations

Updated to address comments

This revision was not accepted when it landed; it landed in state Needs Review.Jun 19 2019, 1:38 AM
Closed by commit rL363771: [RISCV] Add lowering of global TLS addresses (authored by lewis-revill, committed by ). · Explain Why
This revision was automatically updated to reflect the committed changes.