This is an archive of the discontinued LLVM Phabricator instance.

[SPARC][MC] Support more relocation types
ClosedPublic

Authored by LemonBoy on May 16 2021, 4:41 AM.

Details

Summary

This patch introduces support for %hix, %lox, %gdop_hix22, %gdop_lox10 and %gdop.

An extra test is introduced to make sure the fixups are correctly applied.

Diff Detail

Event Timeline

LemonBoy created this revision.May 16 2021, 4:41 AM
LemonBoy requested review of this revision.May 16 2021, 4:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2021, 4:41 AM
jrtc27 requested changes to this revision.May 16 2021, 4:52 AM
jrtc27 added inline comments.
llvm/test/MC/Sparc/sparc-relocations.s
84

Hm, if I understand correctly, this parses fine despite ld only taking one memory operand and one destination register because TLS_LDrr matches this case as we just have def TLSSym : Operand<iPTR>;. That should probably be cleaned up as part of this (possibly split out into a separate patch depending on the route you take).

This revision now requires changes to proceed.May 16 2021, 4:52 AM
jrtc27 added inline comments.May 16 2021, 4:57 AM
llvm/test/MC/Sparc/sparc-relocations.s
74

Although it technically doesn't matter for the test, we should use xor as that's what these relocations are to be used with.

84

Should probably also be ldx given this is 64-bit code, which leads me to conclude that we don't actually have the TableGen definitions for parsing 64-bit TLS ldx either, only 32-bit ld (ie lduw)?

LemonBoy added inline comments.May 16 2021, 6:57 AM
llvm/test/MC/Sparc/sparc-relocations.s
84

I tried to keep the file v8-compatible. Using ldx works because of the TLS_LDXrr pattern, I'll use that over ld if you prefer.

Cleaning the parser is IMO orthogonal to the relocation work, beside the misleading name of the tablegen class it does work as expected.

jrtc27 added inline comments.May 16 2021, 7:03 AM
llvm/test/MC/Sparc/sparc-relocations.s
84

I prefer to not have tests that test things that shouldn't be working but do due to historical accidents. Currently the parser accepts more than it should, but all valid input we claim to support is parsed in a sensible way. This diff changes that so valid input is no longer parsed in a sensible way.

Arguably %gdop should be rejected on anything other than a pointer load (though binutils allows it on _any_ load, not that we should start adding more pseudos so _that_ works IMO, but I would be tempted to predicate TLS_LDrr, and a future GDOP one, on Is32Bit), and I don't want to have an example that's wrong. So yes, please change it to sensible code. Thanks for pointing out TLS_LDXrr, somehow I'd managed to miss that.

LemonBoy updated this revision to Diff 345720.May 16 2021, 12:07 PM

Distinguish %gdop and TLS-relocations applied to ld and ldx at parser level.

Is "tail reloc" a standard term for this? Seems like that could be confused with the notion of tail calls...

llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
865

Make this an enum class (and pass it as a string in TableGen rather than an int)

897–898

These want to be separate kinds so you can stop people using %tie_ld with ldx

llvm/lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp
109

Give it a capital first letter as you're touching the code

llvm/lib/Target/Sparc/SparcISelLowering.h
51

Line up with the others?

llvm/lib/Target/Sparc/SparcInstrInfo.td
1390

Doesn't belong under TLS

Is "tail reloc" a standard term for this?

Not really, the operand matches relocation operands in tail position hence the name. I'm open to suggestions.

llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
865

Will do, I can see that having the Kind as bare numbers can be confusing.

897–898

The idea is to perform only a minimum amount of parsing + backtracking in the parser, this kind of check is better suited to a validateInstruction hook as many other targets do.

llvm/lib/Target/Sparc/SparcInstrInfo.td
1390

Oh well, I completely missed the ASCII-art header. I'll move it.

LemonBoy updated this revision to Diff 345759.May 16 2021, 11:53 PM

Address review comments.

jrtc27 added inline comments.May 23 2021, 8:24 AM
llvm/lib/Target/Sparc/AsmParser/SparcAsmParser.cpp
57

Load isn't a great name given there's Load_TLS. Should say GOT or similar in it.

1531

This should be followed

LemonBoy updated this revision to Diff 348704.May 30 2021, 9:55 AM

Address review comments.

dcederman accepted this revision.Oct 13 2021, 6:37 AM

This looks good to me. Maybe change TailRelocSym to TailOpRelocSym or TailOperandRelocSym to make it more clear that it refers to the tail operand?

brad added a comment.Dec 16 2021, 2:47 AM

LemonBoy?

brad added a comment.Mar 8 2022, 2:14 PM

LemonBoy seems to have disappeared for the time being. Anyway of moving this forward?

Herald added a project: Restricted Project. · View Herald TranscriptMar 8 2022, 2:14 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jun 5 2022, 11:10 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.