Page MenuHomePhabricator

[RISCV] Support assembling TLS add and associated modifiers
ClosedPublic

Authored by lewis-revill on Dec 5 2018, 2:09 PM.

Details

Summary

This patch adds support in the MC layer for parsing and assembling the 4-operand add instruction needed for TLS addressing. This also involves parsing the %tprel_hi, %tprel_lo and %tprel_add operand modifiers.

Diff Detail

Repository
rL LLVM

Event Timeline

lewis-revill created this revision.Dec 5 2018, 2:09 PM

GCC/binutils allows add a0, a0, tp, 0. I've submitted a bug against binutils [1] which attempts to clarify if that should actually be supported or not. Depending on the feedback, this patch's requirement that %tprel_add needs to always be present might need to be revised.
[1] https://sourceware.org/bugzilla/show_bug.cgi?id=23956

rogfer01 added inline comments.Dec 6 2018, 5:13 AM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1139 ↗(On Diff #176879)

I am a bit curious about this function because it looks very similar to RISCVAsmParser::parseOperandWithModifier(OperandVector &Operands), which ends being used when attempting to parse an immediate that starts with %.

My understanding is that isTPRelAddSymbol should filter wrong cases without the need of a custom parser but I think you wanted a more precise diagnostic (as shown in the relevant testcases below)? Is my understanding correct or I am missing other reasons to need a custom parser here?

Thanks!

lewis-revill added inline comments.Dec 6 2018, 10:02 AM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1139 ↗(On Diff #176879)

Yes, I did pretty much copy the function. I will check just using isTPRelAddSymbol with parseOperandWithModifier as the parsing method and see if I can get some decent error messages out of it. If non-%tprel_add operands are valid as @luismarques suggested might be the case then this would be irrelevant anyway.

Rebased with updated dependency.

Use parseOperandWithModifier to parse TPRelAddSymbol.

jrtc27 added a comment.Dec 7 2018, 8:00 AM

GCC/binutils allows add a0, a0, tp, 0. I've submitted a bug against binutils [1] which attempts to clarify if that should actually be supported or not. Depending on the feedback, this patch's requirement that %tprel_add needs to always be present might need to be revised.
[1] https://sourceware.org/bugzilla/show_bug.cgi?id=23956

They've clarified in that bug report that it's not intended and should be fixed, so we don't need to support it.

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
874 ↗(On Diff #177049)

For consistency with Match_InvalidSImm12 (along with my %got_pcrel_hi change and your Match_InvalidTPRelAddSymbol) I'd drop the () from the error message.

Rebased with updated dependencies

Fix bad merge and failing tests

lewis-revill retitled this revision from [RISCV, WIP] Support assembling TLS add and associated modifiers to [RISCV] Support assembling TLS add and associated modifiers.

Rebased (assuming updates to D55279)

Rebased with updated dependency

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 2:10 AM
jrtc27 requested changes to this revision.Feb 5 2019, 9:52 AM
jrtc27 added inline comments.
lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
147 ↗(On Diff #185258)

We also need a fixup_riscv_relax depending on STI.getFeatureBits()[RISCV::FeatureRelax].

251 ↗(On Diff #185258)

Maybe add a comment explaining why we don't expect to see it? Also, perhaps a separate llvm_unreachable that's more useful?

282 ↗(On Diff #185258)

I assume this is coming from your local rebasing of my patches. Note that GOT_HI is *not* a relax candidate so this is incorrect.

This revision now requires changes to proceed.Feb 5 2019, 9:52 AM
jrtc27 added inline comments.Feb 5 2019, 10:14 AM
lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
282 ↗(On Diff #185258)

To clarify why: HI20/LO12 and PCREL_HI20/PCREL_LO12 pairs are relaxed to use GP, CALL/CALL_PLT are relaxed to a single (C.)JAL(R), and TPREL_HI20/TPREL_ADD/TPREL_LO12 is relaxed to a single TP-relative instruction (the LUI and ADDI disappear, and the LO12's instruction's base register is changed to TP).

Since the GOT is almost certainly not within 2K of PC, there's no opportunity to shorten the sequence for PIC. In theory, if you're linking -fPIC code into an executable, you could relax the indirection via the GOT (similarly if you're using -Bsymbolic and the symbol is defined in the output), and also for an executable you could go further and apply the same GP relaxation. However, these are both fairly niche, unlike the other relaxations, hence why they don't exist.

Correct local rebasing of older patch; add fixup_riscv_relax as well as tprel_add where appropriate; seperate reporting of errors for incorrectly expanding a tprel_add fixup.

asb accepted this revision.Apr 2 2019, 5:55 AM

Hi Lewis, this looks good to me - thanks! Do you want to request commit access as per the developer policy so you can commit yourself?

@asb thanks, I've just requested commit access!

This revision was not accepted when it landed; it landed in state Needs Review.Apr 4 2019, 7:12 AM
This revision was automatically updated to reflect the committed changes.