[PATCH] D102575: [SPARC][MC] Support more relocation types

Sun May 16 07:03:29 PDT 2021

jrtc27 added inline comments.

================
Comment at: llvm/test/MC/Sparc/sparc-relocations.s:84
+
+        ! CHECK: ld [%l7+%l1], %l2, %gdop(sym) ! encoding: [0xe4,0x05,0xc0,0x11]
+        ! CHECK-NEXT:                          ! fixup A - offset: 0, value: %gdop(sym), kind: fixup_sparc_gotdata_op
----------------
LemonBoy wrote:
> jrtc27 wrote:
> > jrtc27 wrote:
> > > 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).
> > 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`)?
> 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.
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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102575/new/

https://reviews.llvm.org/D102575