Discussion:
[PATCH] RISC-V: Add %got_pcrel_hi operator
James Clarke
2018-11-07 18:27:41 UTC
Permalink
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.
---
gas/config/tc-riscv.c | 1 +
gas/testsuite/gas/riscv/no-relax-reloc.d | 4 +++-
gas/testsuite/gas/riscv/no-relax-reloc.s | 3 +++
gas/testsuite/gas/riscv/relax-reloc.d | 7 +++++--
gas/testsuite/gas/riscv/relax-reloc.s | 3 +++
5 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/gas/config/tc-riscv.c b/gas/config/tc-riscv.c
index 987377ae81..5f4a1bbe83 100644
--- a/gas/config/tc-riscv.c
+++ b/gas/config/tc-riscv.c
@@ -1286,6 +1286,7 @@ static const struct percent_op_match percent_op_utype[] =
{
{"%tprel_hi", BFD_RELOC_RISCV_TPREL_HI20},
{"%pcrel_hi", BFD_RELOC_RISCV_PCREL_HI20},
+ {"%got_pcrel_hi", BFD_RELOC_RISCV_GOT_HI20},
{"%tls_ie_pcrel_hi", BFD_RELOC_RISCV_TLS_GOT_HI20},
{"%tls_gd_pcrel_hi", BFD_RELOC_RISCV_TLS_GD_HI20},
{"%hi", BFD_RELOC_RISCV_HI20},
diff --git a/gas/testsuite/gas/riscv/no-relax-reloc.d b/gas/testsuite/gas/riscv/no-relax-reloc.d
index 62f28e0927..c2ca1aa6e7 100644
--- a/gas/testsuite/gas/riscv/no-relax-reloc.d
+++ b/gas/testsuite/gas/riscv/no-relax-reloc.d
@@ -9,4 +9,6 @@ RELOCATION RECORDS FOR .*
0+4 R_RISCV_LO12_I.*
0+8 R_RISCV_PCREL_HI20.*
0+c R_RISCV_PCREL_LO12_I.*
-0+10 R_RISCV_CALL.*
+0+10 R_RISCV_GOT_HI20.*
+0+14 R_RISCV_PCREL_LO12_I.*
+0+18 R_RISCV_CALL.*
diff --git a/gas/testsuite/gas/riscv/no-relax-reloc.s b/gas/testsuite/gas/riscv/no-relax-reloc.s
index 7f1a484fc2..d63852d08e 100644
--- a/gas/testsuite/gas/riscv/no-relax-reloc.s
+++ b/gas/testsuite/gas/riscv/no-relax-reloc.s
@@ -5,4 +5,7 @@ target:
.LA0: auipc a5,%pcrel_hi(bar)
lw a0,%pcrel_lo(.LA0)(a5)

+ .LA1: auipc a5,%got_pcrel_hi(baz)
+ lw a0,%pcrel_lo(.LA1)(a5)
+
call target
diff --git a/gas/testsuite/gas/riscv/relax-reloc.d b/gas/testsuite/gas/riscv/relax-reloc.d
index f5f592ce03..623218ec5d 100644
--- a/gas/testsuite/gas/riscv/relax-reloc.d
+++ b/gas/testsuite/gas/riscv/relax-reloc.d
@@ -13,5 +13,8 @@ RELOCATION RECORDS FOR .*
0+8 R_RISCV_RELAX.*
0+c R_RISCV_PCREL_LO12_I.*
0+c R_RISCV_RELAX.*
-0+10 R_RISCV_CALL.*
-0+10 R_RISCV_RELAX.*
+0+10 R_RISCV_GOT_HI20.*
+0+14 R_RISCV_PCREL_LO12_I.*
+0+14 R_RISCV_RELAX.*
+0+18 R_RISCV_CALL.*
+0+18 R_RISCV_RELAX.*
diff --git a/gas/testsuite/gas/riscv/relax-reloc.s b/gas/testsuite/gas/riscv/relax-reloc.s
index 7f1a484fc2..d63852d08e 100644
--- a/gas/testsuite/gas/riscv/relax-reloc.s
+++ b/gas/testsuite/gas/riscv/relax-reloc.s
@@ -5,4 +5,7 @@ target:
.LA0: auipc a5,%pcrel_hi(bar)
lw a0,%pcrel_lo(.LA0)(a5)

+ .LA1: auipc a5,%got_pcrel_hi(baz)
+ lw a0,%pcrel_lo(.LA1)(a5)
+
call target
--
2.17.1
Jim Wilson
2018-11-08 00:40:34 UTC
Permalink
Post by James Clarke
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 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-***@riscv.org is probably sufficient.

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.

Do you have a copyright assignment for binutils? I found ones for mach,
hurd, and glibc. I don't see an obvious binutils one.

Otherwise this looks OK to me.

Jim
James Clarke
2018-11-08 01:22:18 UTC
Permalink
Post by Jim Wilson
Post by James Clarke
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 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
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.
Post by Jim Wilson
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.
Post by Jim Wilson
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
Post by Jim Wilson
Otherwise this looks OK to me.
Jim
Palmer Dabbelt
2018-11-08 02:10:07 UTC
Permalink
Post by James Clarke
Post by Jim Wilson
Post by James Clarke
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.
Post by James Clarke
Post by Jim Wilson
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
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.
Post by James Clarke
Post by Jim Wilson
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.
Post by James Clarke
Post by Jim Wilson
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
Post by Jim Wilson
Otherwise this looks OK to me.
Jim
Thanks!
Andrew Waterman
2018-11-08 02:18:24 UTC
Permalink
Post by Palmer Dabbelt
Post by James Clarke
Post by Jim Wilson
Post by James Clarke
This operator allows compilers to expand the "la" pseduo-instruction
and
Post by James Clarke
Post by Jim Wilson
Post by James Clarke
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
Post by James Clarke
Post by Jim Wilson
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.
Post by James Clarke
Post by Jim Wilson
It is probably best if there is a public proposal somewhere on the
RISC-V side
Post by James Clarke
Post by Jim Wilson
instead of unilaterally changing assembler syntax, particularly given
that it
Post by James Clarke
Post by Jim Wilson
is part of the psABI (even though it probably shouldn't be). Someone
on the
Post by James Clarke
Post by Jim Wilson
LLVM side for instance might want to offer an opinion. An issue filed
with the
probably
Post by James Clarke
Post by Jim Wilson
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
Post by James Clarke
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.
Post by James Clarke
Post by Jim Wilson
Maybe the operator could just be %got_hi? Not clear why we need the
pcrel in
Post by James Clarke
Post by Jim Wilson
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
Post by James Clarke
%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,
Post by James Clarke
whereas %tls_{ie,gd}_pcrel_hi use %pcrel_lo for the second half, so
given we
Post by James Clarke
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.
Post by Palmer Dabbelt
Post by James Clarke
Post by Jim Wilson
Do you have a copyright assignment for binutils? I found ones for
mach, hurd,
Post by James Clarke
Post by Jim Wilson
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
Post by James Clarke
way, I've just sent in a request for the binutils (and gcc while I'm at
it)
Post by James Clarke
forms.
James
Post by Jim Wilson
Otherwise this looks OK to me.
Jim
Thanks!
James Clarke
2018-11-08 11:35:52 UTC
Permalink
Post by Jim Wilson
Post by James Clarke
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.
That's a discussion for later. I've opened [1] against riscv-asm-manual, so
let's take this discussion there.

James
James Clarke
2018-11-08 23:28:09 UTC
Permalink
Post by James Clarke
Post by Jim Wilson
Post by James Clarke
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.
That's a discussion for later. I've opened [1] against riscv-asm-manual, so
let's take this discussion there.
James
[1] https://github.com/riscv/riscv-asm-manual/pull/16
Jim Wilson
2018-11-14 00:09:57 UTC
Permalink
Post by James Clarke
Post by James Clarke
That's a discussion for later. I've opened [1] against riscv-asm-manual, so
let's take this discussion there.
James
[1] https://github.com/riscv/riscv-asm-manual/pull/16
There has been no contrary opinions offered there, so I approved the
riscv-asm-manual patch, though I don't have write access to commit it.
I see your binutils/gcc assignment came in over the weekend, so that
is good too. I'm approving your binutils patch. I don't know if you
have write access to binutils. Do you want me to commit the binutils
patch? I do have write access for that.

Jim
James Clarke
2018-11-18 15:44:44 UTC
Permalink
Post by Jim Wilson
Post by James Clarke
Post by James Clarke
That's a discussion for later. I've opened [1] against riscv-asm-manual, so
let's take this discussion there.
James
[1] https://github.com/riscv/riscv-asm-manual/pull/16
I see your binutils/gcc assignment came in over the weekend, so that
is good too.
I've submitted the paperwork but not yet had a confirmation/received
countersigned copies.
Post by Jim Wilson
I'm approving your binutils patch. I don't know if you
have write access to binutils. Do you want me to commit the binutils
patch? I do have write access for that.
I don't have commit rights to binutils (nor gcc), so please do if (/when) the
copyright situation is fine.

Thanks,
James
Jim Wilson
2018-11-19 23:53:22 UTC
Permalink
Post by James Clarke
I've submitted the paperwork but not yet had a confirmation/received
countersigned copies.
My mistake, the email I saw was for the unsigned assignment. I will
wait for this to appear in the copyright list file at the FSF, and
then commit.

Jim

Palmer Dabbelt
2018-11-08 16:57:26 UTC
Permalink
Post by James Clarke
Post by Jim Wilson
Post by James Clarke
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.
That's a discussion for later. I've opened [1] against riscv-asm-manual, so
let's take this discussion there.
Thanks!
Palmer Dabbelt
2018-11-14 23:43:19 UTC
Permalink
Post by Jim Wilson
Post by James Clarke
Post by James Clarke
That's a discussion for later. I've opened [1] against riscv-asm-manual, so
let's take this discussion there.
James
[1] https://github.com/riscv/riscv-asm-manual/pull/16
There has been no contrary opinions offered there, so I approved the
riscv-asm-manual patch, though I don't have write access to commit it.
You should now.
Post by Jim Wilson
I see your binutils/gcc assignment came in over the weekend, so that
is good too. I'm approving your binutils patch. I don't know if you
have write access to binutils. Do you want me to commit the binutils
patch? I do have write access for that.
Loading...