[PATCH] RISC-V: Add %got_pcrel_hi operator

[Andrew Waterman]

On Wed, Nov 7, 2018 at 6:10 PM Palmer Dabbelt <palmer@sifive.com> wrote:

> On Wed, 07 Nov 2018 17:22:18 PST (-0800), jrtc27@jrtc27.com wrote:
> > On 8 Nov 2018, at 00:40, Jim Wilson <jimw@sifive.com> wrote:
> >> On 11/7/18 10:27 AM, James Clarke wrote:
> >>> This operator allows compilers to expand the "la" pseduo-instruction
> and
> >>> potentially split up the two instructions.
> >>> gas/
> >>>     * config/tc-riscv.c (percent_op_utype): Add entry for a new
> >>>     %got_pcrel_hi operator.
> >>>     * testsuite/gas/riscv/no-relax-reloc.d: Update for new
> >>>     %got_pcrel_hi operator test.
> >>>     * testsuite/gas/riscv/no-relax-reloc.s: Add a %got_pcrel_hi
> >>>     operator test.
> >>>     * testsuite/gas/riscv/relax-reloc.d: Update for new
> >>>     %got_pcrel_hi operator test.
> >>>     * testsuite/gas/riscv/relax-reloc.s: Add a %got_pcrel_hi
> >>>     operator test.
> >>
> >> We have a list of assembler operators in the psABI document
> >>    https://github.com/riscv/riscv-elf-psabi-doc
> >> We also have a list of assembler operators in the assembler manual
> >>    https://github.com/riscv/riscv-asm-manual
> >> This looks a little redundant.  It isn't clear why assembler syntax is
> >> mentioned in the psABI.  It doesn't seem relevant to that.  Anyways,
> these
> >> should be updated.
>
> It's a bit off topic, but IIRC we didn't have an assembly manual when the
> psABI
> doc was started so it ended up in there.  It doesn't make a whole lot of
> sense,
> I'd be OK dropping the assembler syntax from the psABI doc.
>
> >> It is probably best if there is a public proposal somewhere on the
> RISC-V side
> >> instead of unilaterally changing assembler syntax, particularly given
> that it
> >> is part of the psABI (even though it probably shouldn't be).  Someone
> on the
> >> LLVM side for instance might want to offer an opinion.  An issue filed
> with the
> >> psABI or assembler manual, or an email sent to sw-dev@riscv.org is
> probably
> >> sufficient.
> >
> > Ok, makes sense to get it standardised. I'll file an issue/PR against
> > riscv-asm-manual as that seems the more logical place of the two
> repositories
> > to have the discussion, and reach out to the mailing list if nobody
> bites.
>
> Specifically: we want to make sure the LLVM syntax stays in sync with the
> GCC
> syntax.  Alex (To'd) is the person to talk to on the LLVM side, but our
> general
> practice is to propose assembler syntax additions in the spec and then
> point it
> out on sw-dev for good measure.
>
> >> Maybe the operator could just be %got_hi?  Not clear why we need the
> pcrel in
> >> there.  The reloc itself is just R_RISCV_GOT_HI20.
> >
> > I wasn't sure which to go for, as there's some inconsistency. We have
> > %tls_{ie,gd}_pcrel_hi that map to R_RISCV_TLS_GOT_HI20, which would lead
> to
> > %got_pcrel_hi being the logical choice, but also %tprel_hi mapping to
> > R_RISCV_TPREL_HI20. However, %tprel_hi uses %tprel_lo for the second
> half,
> > whereas %tls_{ie,gd}_pcrel_hi use %pcrel_lo for the second half, so
> given we
> > also use %pcrel_lo here I decided that it made sense to match that.
>
> Andrew (also To'd) would know for sure, as this predates my involvement,
> but
> I'm pretty sure these all came from MIPS and then just sort of evolved
> until
> they did something useful.  At some point the relocations were all
> redesigned
> for RISC-V -- that one I was around for, and I'm pretty sure we decided
> not to
> mess with the assembler syntax which is probably why the mappings appear
> nonsensical.
>
> Given that MIPS has "%got_hi", I'd also go with "%got_hi" -- at least that
> way
> we're consistently inconsistent :).  I'm fine either way, though.


The inconsistency is surely my fault - I don’t think we can blame MIPS this
time.

got_pcrel_hi seems better to me, but it’s not a strong opinion.


>
> >> Do you have a copyright assignment for binutils?  I found ones for
> mach, hurd,
> >> and glibc.  I don't see an obvious binutils one.
> >
> > No, it's something I should have done years ago. I've had small patches
> > accepted, but I don't know whether this is regarded as small enough.
> Either
> > way, I've just sent in a request for the binutils (and gcc while I'm at
> it)
> > forms.
> >
> > James
> >
> >> Otherwise this looks OK to me.
> >>
> >> Jim
>
> Thanks!
>