| Message ID | cover.1691561509.git.research_trasio@irq.a4lg.com |
|---|---|
| Headers | show |
| Series | RISC-V: __builtin_riscv_pause for all environment | expand |
On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote: > Hello, > > I found that a built-in function "__builtin_riscv_pause" is not usable > unless we enable the 'Zihintpause' extension explicitly (still, this > built-in exists EVEN IF the 'Zihintpause' extension is disabled). > > Contents of a.c: > >> void sample(void) >> { >> __builtin_riscv_pause(); >> } > > > Compiling with the 'Zihintpause' extension is fine. > >> $ riscv64-unknown-elf-gcc -O2 -march=rv64i_zihintpause -mabi=lp64 -c a.c > > > However, compiling without the 'Zihintpause' causes an assembler error > (tested with GNU Binutils 2.41): > >> $ riscv64-unknown-elf-gcc -O2 -march=rv64i -mabi=lp64 -c a.c >> /tmp/ccFjacAz.s: Assembler messages: >> /tmp/ccFjacAz.s:11: Error: unrecognized opcode `pause', extension `zihintpause' required > > > This is because: > > 1. GCC does not handle the 'Zihintpause' extension and > 2. "riscv_pause" (insn) unconditionally emits "pause" even if the > assembler does not accept it (when the extension is disabled). > > > This patch set (PATCH 1/2) resolves this issue by: > > 1. Handling the 'Zihintpause' extension and > 2. Splitting the "__builtin_riscv_pause" implementation > depending on the existence of the 'Zihintpause' extension. > > Because a released version of GCC defines "__builtin_riscv_pause" > unconditionally, I chose to define no-'Zihintpause' version. > > There is another option to unconditionally emit ".insn 0x0100000f" > (the machine code of "pause") but I didn't because I wanted to improve the > diagnostics (e.g. *.s output). > > I also fixed the description of this built-in function (in PATCH 2/2). > > > I'm not sure whether this is a good method to split the implementation > depending on the 'Zihintpause' extension. Other than that, I believe that > this is okay and approval is appreciated. > > Note that because I assigned copyright of GCC contribution to FSF, I didn't > attach "Signed-off-by" tag. Tell me if I need it. I'd tend to think we do not want to expose the intrinsic unless the right extensions are enabled -- even though the encoding is a no-op and we could emit it as a .insn. If others think otherwise, I'll go with the consensus opinion. So let's hold off a bit until others have chimed in. Thanks, jeff
On 2023/08/10 5:05, Jeff Law wrote: > > > On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote: >> Hello, >> >> I found that a built-in function "__builtin_riscv_pause" is not usable >> unless we enable the 'Zihintpause' extension explicitly (still, this >> built-in exists EVEN IF the 'Zihintpause' extension is disabled). >> >> Contents of a.c: >> >>> void sample(void) >>> { >>> __builtin_riscv_pause(); >>> } >> >> >> Compiling with the 'Zihintpause' extension is fine. >> >>> $ riscv64-unknown-elf-gcc -O2 -march=rv64i_zihintpause -mabi=lp64 -c a.c >> >> >> However, compiling without the 'Zihintpause' causes an assembler error >> (tested with GNU Binutils 2.41): >> >>> $ riscv64-unknown-elf-gcc -O2 -march=rv64i -mabi=lp64 -c a.c >>> /tmp/ccFjacAz.s: Assembler messages: >>> /tmp/ccFjacAz.s:11: Error: unrecognized opcode `pause', extension >>> `zihintpause' required >> >> >> This is because: >> >> 1. GCC does not handle the 'Zihintpause' extension and >> 2. "riscv_pause" (insn) unconditionally emits "pause" even if the >> assembler does not accept it (when the extension is disabled). >> >> >> This patch set (PATCH 1/2) resolves this issue by: >> >> 1. Handling the 'Zihintpause' extension and >> 2. Splitting the "__builtin_riscv_pause" implementation >> depending on the existence of the 'Zihintpause' extension. >> >> Because a released version of GCC defines "__builtin_riscv_pause" >> unconditionally, I chose to define no-'Zihintpause' version. >> >> There is another option to unconditionally emit ".insn 0x0100000f" >> (the machine code of "pause") but I didn't because I wanted to improve >> the >> diagnostics (e.g. *.s output). >> >> I also fixed the description of this built-in function (in PATCH 2/2). >> >> >> I'm not sure whether this is a good method to split the implementation >> depending on the 'Zihintpause' extension. Other than that, I believe >> that >> this is okay and approval is appreciated. >> >> Note that because I assigned copyright of GCC contribution to FSF, I >> didn't >> attach "Signed-off-by" tag. Tell me if I need it. > I'd tend to think we do not want to expose the intrinsic unless the > right extensions are enabled -- even though the encoding is a no-op and > we could emit it as a .insn. I think that makes sense. The only reason I implemented the no-'Zihintpause' version is because GCC 13 implemented the built-in unconditionally. If the compatibility breakage is considered minimum (I don't know, though), I'm ready to submit 'Zihintpause'-only version of this patch set. Thanks, Tsukasa > > If others think otherwise, I'll go with the consensus opinion. So let's > hold off a bit until others have chimed in. > > Thanks, > jeff >
On 8/9/23 16:39, Tsukasa OI wrote: > On 2023/08/10 5:05, Jeff Law wrote: >> I'd tend to think we do not want to expose the intrinsic unless the >> right extensions are enabled -- even though the encoding is a no-op and >> we could emit it as a .insn. > > I think that makes sense. The only reason I implemented the > no-'Zihintpause' version is because GCC 13 implemented the built-in > unconditionally. If the compatibility breakage is considered minimum (I > don't know, though), I'm ready to submit 'Zihintpause'-only version of > this patch set. While it's a compatibility break I don't think we have a need to preserve this kind of compatibility. I suspect anyone using __builtin_riscv_pause was probably already turning on Zihintpause and if they weren't they should have been :-0 I'm sure we'll kick this around in the Tuesday meeting and hopefully make a decision about the desired direction. You're obviously welcome to join if you're inclined. Let me know if you need an invite. jeff
On Fri, 11 Aug 2023 16:30:29 PDT (-0700), gcc-patches@gcc.gnu.org wrote: > > > On 8/9/23 16:39, Tsukasa OI wrote: >> On 2023/08/10 5:05, Jeff Law wrote: > >>> I'd tend to think we do not want to expose the intrinsic unless the >>> right extensions are enabled -- even though the encoding is a no-op and >>> we could emit it as a .insn. >> >> I think that makes sense. The only reason I implemented the >> no-'Zihintpause' version is because GCC 13 implemented the built-in >> unconditionally. If the compatibility breakage is considered minimum (I >> don't know, though), I'm ready to submit 'Zihintpause'-only version of >> this patch set. > While it's a compatibility break I don't think we have a need to > preserve this kind of compatibility. I suspect anyone using > __builtin_riscv_pause was probably already turning on Zihintpause and if > they weren't they should have been :-0 I agree it's fine to just call this a bug: the builtin wasn't doing anything on non-Zihintpause systems anyway, so it's not like it could have been all that useful. > I'm sure we'll kick this around in the Tuesday meeting and hopefully > make a decision about the desired direction. You're obviously welcome > to join if you're inclined. Let me know if you need an invite. > > jeff
On 2023/08/12 8:30, Jeff Law wrote: > > > On 8/9/23 16:39, Tsukasa OI wrote: >> On 2023/08/10 5:05, Jeff Law wrote: > >>> I'd tend to think we do not want to expose the intrinsic unless the >>> right extensions are enabled -- even though the encoding is a no-op and >>> we could emit it as a .insn. >> >> I think that makes sense. The only reason I implemented the >> no-'Zihintpause' version is because GCC 13 implemented the built-in >> unconditionally. If the compatibility breakage is considered minimum (I >> don't know, though), I'm ready to submit 'Zihintpause'-only version of >> this patch set. > While it's a compatibility break I don't think we have a need to > preserve this kind of compatibility. I suspect anyone using > __builtin_riscv_pause was probably already turning on Zihintpause and if > they weren't they should have been :-0 > > > I'm sure we'll kick this around in the Tuesday meeting and hopefully > make a decision about the desired direction. You're obviously welcome > to join if you're inclined. Let me know if you need an invite. > > jeff > I'll not be able to attend that meeting due to Japanese religious events around Aug 13-16 (it may not be impossible but at least difficult) but look forward seeing that some conclusion is made. I leave two patch sets corresponding two options so in either case, we can apply a fix after the conclusion is made. (1) __builtin_riscv_pause for 'Zihintpause'-only <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626912.html> (2) __builtin_riscv_pause for all <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626909.html> Thanks, Tsukasa
On Sat, 12 Aug 2023 at 01:31, Jeff Law via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > On 8/9/23 16:39, Tsukasa OI wrote: > > On 2023/08/10 5:05, Jeff Law wrote: > > >> I'd tend to think we do not want to expose the intrinsic unless the > >> right extensions are enabled -- even though the encoding is a no-op and > >> we could emit it as a .insn. > > > > I think that makes sense. The only reason I implemented the > > no-'Zihintpause' version is because GCC 13 implemented the built-in > > unconditionally. If the compatibility breakage is considered minimum (I > > don't know, though), I'm ready to submit 'Zihintpause'-only version of > > this patch set. > While it's a compatibility break I don't think we have a need to > preserve this kind of compatibility. I suspect anyone using > __builtin_riscv_pause was probably already turning on Zihintpause and if > they weren't they should have been :-0 > > > I'm sure we'll kick this around in the Tuesday meeting and hopefully > make a decision about the desired direction. You're obviously welcome > to join if you're inclined. Let me know if you need an invite. The original discussion (and I believe that Andrew was the decisive voice in the end) came to the conclusion that—given that pause is a true hint—it could always be enabled. We had originally expected to enable it only if Zihintpause was part of the target architecture, but viewing it as "just a name for an already existing pure hint" also made sense. Note that on systems that don't implement Zihintpause, the hint is guarantueed to not have an architectural effect. That said, I don't really have a strong leaning one way or another. Philipp.
On Sun, Aug 13, 2023 at 12:53 PM Philipp Tomsich <philipp.tomsich@vrull.eu> wrote: > > On Sat, 12 Aug 2023 at 01:31, Jeff Law via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: > > > > > > > > On 8/9/23 16:39, Tsukasa OI wrote: > > > On 2023/08/10 5:05, Jeff Law wrote: > > > > >> I'd tend to think we do not want to expose the intrinsic unless the > > >> right extensions are enabled -- even though the encoding is a no-op and > > >> we could emit it as a .insn. > > > > > > I think that makes sense. The only reason I implemented the > > > no-'Zihintpause' version is because GCC 13 implemented the built-in > > > unconditionally. If the compatibility breakage is considered minimum (I > > > don't know, though), I'm ready to submit 'Zihintpause'-only version of > > > this patch set. > > While it's a compatibility break I don't think we have a need to > > preserve this kind of compatibility. I suspect anyone using > > __builtin_riscv_pause was probably already turning on Zihintpause and if > > they weren't they should have been :-0 > > > > > > I'm sure we'll kick this around in the Tuesday meeting and hopefully > > make a decision about the desired direction. You're obviously welcome > > to join if you're inclined. Let me know if you need an invite. > > The original discussion (and I believe that Andrew was the decisive > voice in the end) came to the conclusion that—given that pause is a > true hint—it could always be enabled. I continue to think that, since it's semantically valid to execute a HINT on any implementation, there's little utility in ever rejecting the HINT builtins, or in rejecting explicit HINTs in asm, irrespective of -march. But ultimately it isn't a big deal either way. > We had originally expected to enable it only if Zihintpause was part > of the target architecture, but viewing it as "just a name for an > already existing pure hint" also made sense. > Note that on systems that don't implement Zihintpause, the hint is > guarantueed to not have an architectural effect. > > That said, I don't really have a strong leaning one way or another. > Philipp.
On 8/11/23 18:20, Tsukasa OI wrote: >> > > I'll not be able to attend that meeting due to Japanese religious events > around Aug 13-16 (it may not be impossible but at least difficult) but > look forward seeing that some conclusion is made. No problem. We hold that meeting weekly to work through any outstanding patches related to RISC-V. You're always welcome to attend if you want. > > I leave two patch sets corresponding two options so in either case, we > can apply a fix after the conclusion is made. > > (1) __builtin_riscv_pause for 'Zihintpause'-only > <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626912.html> > (2) __builtin_riscv_pause for all > <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626909.html> Thanks. It's not clear what direction we'll take on this, but having a patchkit for both potential outcomes is definitely helpful. jeff
On 8/13/23 13:52, Philipp Tomsich wrote: > On Sat, 12 Aug 2023 at 01:31, Jeff Law via Gcc-patches > <gcc-patches@gcc.gnu.org> wrote: >> >> >> >> On 8/9/23 16:39, Tsukasa OI wrote: >>> On 2023/08/10 5:05, Jeff Law wrote: >> >>>> I'd tend to think we do not want to expose the intrinsic unless the >>>> right extensions are enabled -- even though the encoding is a no-op and >>>> we could emit it as a .insn. >>> >>> I think that makes sense. The only reason I implemented the >>> no-'Zihintpause' version is because GCC 13 implemented the built-in >>> unconditionally. If the compatibility breakage is considered minimum (I >>> don't know, though), I'm ready to submit 'Zihintpause'-only version of >>> this patch set. >> While it's a compatibility break I don't think we have a need to >> preserve this kind of compatibility. I suspect anyone using >> __builtin_riscv_pause was probably already turning on Zihintpause and if >> they weren't they should have been :-0 >> >> >> I'm sure we'll kick this around in the Tuesday meeting and hopefully >> make a decision about the desired direction. You're obviously welcome >> to join if you're inclined. Let me know if you need an invite. > > The original discussion (and I believe that Andrew was the decisive > voice in the end) came to the conclusion that—given that pause is a > true hint—it could always be enabled. > We had originally expected to enable it only if Zihintpause was part > of the target architecture, but viewing it as "just a name for an > already existing pure hint" also made sense. > Note that on systems that don't implement Zihintpause, the hint is > guarantueed to not have an architectural effect. > > That said, I don't really have a strong leaning one way or another. > Philipp. I don't have a strong opinion either way and I certainly see both sides of the argument. It sounds like the current situation is by design; knowing that now I would tend to lean towards keeping status quo, which would mean going with Tsukasa's first patch or something similar. We'll certainly discuss on the call in a half-hour or so. jeff