[SPARC][MC] Support more relocation types

[LemonBoy]

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.

[jrtc27]

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).

[jrtc27]

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]

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]

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]

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

[jrtc27]

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

866

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

898–899

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

53

Line up with the others?

llvm/lib/Target/Sparc/SparcInstrInfo.td

1420

Doesn't belong under TLS

[LemonBoy]

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

866

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

898–899

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

1420

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

[LemonBoy]

Address review comments.

[jrtc27]

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

58

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

1534

This should be followed

[LemonBoy]

Address review comments.

[LemonBoy]

Ping

[brad]

ping

[dcederman]

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]

ping.

[brad]

LemonBoy?

[brad]

ping.

[brad]

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