* [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. @ 2023-05-29 11:01 Jin Ma 2023-05-29 12:46 ` Jeff Law 0 siblings, 1 reply; 11+ messages in thread From: Jin Ma @ 2023-05-29 11:01 UTC (permalink / raw) To: gcc-patches Cc: jeffreyalaw, richard.sandiford, kito.cheng, christoph.muellner, jinma.contrib, Jin Ma Reference: https://github.com/gcc-mirror/gcc/commit/d0bc0cb66bcb0e6a5a5a31a9e900e8ccc98e34e5 RISC-V should also be implemented to handle no_insn patterns for pipelining. gcc/ChangeLog: * config/riscv/riscv.cc (riscv_sched_variable_issue): New function. (TARGET_SCHED_VARIABLE_ISSUE): New macro. --- gcc/config/riscv/riscv.cc | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index 3954fc07a8b..559fa9cd7e0 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -6225,6 +6225,24 @@ riscv_issue_rate (void) return tune_param->issue_rate; } +/* Implement TARGET_SCHED_VARIABLE_ISSUE. */ + +static int +riscv_sched_variable_issue (FILE *, int, rtx_insn *insn, int more) +{ + if (DEBUG_INSN_P (insn)) + return more; + + rtx_code code = GET_CODE (PATTERN (insn)); + if (code == USE || code == CLOBBER) + return more; + + if (get_attr_type (insn) == TYPE_UNKNOWN) + return more; + + return more - 1; +} + /* Auxiliary function to emit RISC-V ELF attribute. */ static void riscv_emit_attribute () @@ -7671,6 +7689,9 @@ riscv_vectorize_related_mode (machine_mode vector_mode, scalar_mode element_mode #undef TARGET_SCHED_ISSUE_RATE #define TARGET_SCHED_ISSUE_RATE riscv_issue_rate +#undef TARGET_SCHED_VARIABLE_ISSUE +#define TARGET_SCHED_VARIABLE_ISSUE riscv_sched_variable_issue + #undef TARGET_FUNCTION_OK_FOR_SIBCALL #define TARGET_FUNCTION_OK_FOR_SIBCALL riscv_function_ok_for_sibcall -- 2.17.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-05-29 11:01 [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE Jin Ma @ 2023-05-29 12:46 ` Jeff Law 2023-08-09 19:56 ` Jeff Law 0 siblings, 1 reply; 11+ messages in thread From: Jeff Law @ 2023-05-29 12:46 UTC (permalink / raw) To: Jin Ma, gcc-patches Cc: richard.sandiford, kito.cheng, christoph.muellner, jinma.contrib On 5/29/23 05:01, Jin Ma wrote: > Reference: https://github.com/gcc-mirror/gcc/commit/d0bc0cb66bcb0e6a5a5a31a9e900e8ccc98e34e5 > > RISC-V should also be implemented to handle no_insn patterns for pipelining. > > gcc/ChangeLog: > > * config/riscv/riscv.cc (riscv_sched_variable_issue): New function. > (TARGET_SCHED_VARIABLE_ISSUE): New macro. > --- > gcc/config/riscv/riscv.cc | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc > index 3954fc07a8b..559fa9cd7e0 100644 > --- a/gcc/config/riscv/riscv.cc > +++ b/gcc/config/riscv/riscv.cc > @@ -6225,6 +6225,24 @@ riscv_issue_rate (void) > return tune_param->issue_rate; > } > > +/* Implement TARGET_SCHED_VARIABLE_ISSUE. */ > + > +static int > +riscv_sched_variable_issue (FILE *, int, rtx_insn *insn, int more) > +{ > + if (DEBUG_INSN_P (insn)) > + return more; > + > + rtx_code code = GET_CODE (PATTERN (insn)); > + if (code == USE || code == CLOBBER) > + return more; > + > + if (get_attr_type (insn) == TYPE_UNKNOWN) > + return more; > + > + return more - 1; > +} The problem is that INSN is *much* more likely to be a real instruction that takes real resources, even if it is TYPE_UNKNOWN. TYPE_UNKNOWN here is actually an indicator of what I would consider a bug in the backend, specifically that we have INSNs that do not provide a mapping for the schedulers to suitable types. With that in mind I'd much rather get to the point where we can do something like this for TYPE_UNKNOWN: type = get_attr_type (insn); gcc_assert (type != TYPE_UNKNOWN); That way if we ever encounter a TYPE_UNKNOWN during development, we can fix it in the md files in a sensible manner. I don't know if we are close to being able to do that. We fixed a ton of stuff in bitmanip.md, but I don't think there's been a thorough review of the port to find other instances of TYPE_UNKNOWN INSNs. The other thing if this code probably wants to handle GHOST type instructions. While GHOST is used for instructions which generate no code, it might seem they should return "more" as those INSNs take no resources. But GHOST is actually used for things like the blockage insn which should end a cycle from an issue standpoint. So the right handling of a GHOST is something like this: if (type == TYPE_GHOST) return 0; Jeff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-05-29 12:46 ` Jeff Law @ 2023-08-09 19:56 ` Jeff Law 2023-08-11 2:12 ` Jin Ma 0 siblings, 1 reply; 11+ messages in thread From: Jeff Law @ 2023-08-09 19:56 UTC (permalink / raw) To: Jin Ma, gcc-patches Cc: richard.sandiford, kito.cheng, christoph.muellner, jinma.contrib [-- Attachment #1: Type: text/plain, Size: 2833 bytes --] On 5/29/23 06:46, Jeff Law wrote: > > > On 5/29/23 05:01, Jin Ma wrote: >> Reference: >> https://github.com/gcc-mirror/gcc/commit/d0bc0cb66bcb0e6a5a5a31a9e900e8ccc98e34e5 >> >> RISC-V should also be implemented to handle no_insn patterns for >> pipelining. >> >> gcc/ChangeLog: >> >> * config/riscv/riscv.cc (riscv_sched_variable_issue): New function. >> (TARGET_SCHED_VARIABLE_ISSUE): New macro. >> --- >> gcc/config/riscv/riscv.cc | 21 +++++++++++++++++++++ >> 1 file changed, 21 insertions(+) >> >> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc >> index 3954fc07a8b..559fa9cd7e0 100644 >> --- a/gcc/config/riscv/riscv.cc >> +++ b/gcc/config/riscv/riscv.cc >> @@ -6225,6 +6225,24 @@ riscv_issue_rate (void) >> return tune_param->issue_rate; >> } >> +/* Implement TARGET_SCHED_VARIABLE_ISSUE. */ >> + >> +static int >> +riscv_sched_variable_issue (FILE *, int, rtx_insn *insn, int more) >> +{ >> + if (DEBUG_INSN_P (insn)) >> + return more; >> + >> + rtx_code code = GET_CODE (PATTERN (insn)); >> + if (code == USE || code == CLOBBER) >> + return more; >> + >> + if (get_attr_type (insn) == TYPE_UNKNOWN) >> + return more; >> + >> + return more - 1; >> +} > The problem is that INSN is *much* more likely to be a real instruction > that takes real resources, even if it is TYPE_UNKNOWN. > TYPE_UNKNOWN here is actually an indicator of what I would consider a > bug in the backend, specifically that we have INSNs that do not provide > a mapping for the schedulers to suitable types. > > With that in mind I'd much rather get to the point where we can do > something like this for TYPE_UNKNOWN: > > type = get_attr_type (insn); > gcc_assert (type != TYPE_UNKNOWN); > > That way if we ever encounter a TYPE_UNKNOWN during development, we can > fix it in the md files in a sensible manner. I don't know if we are > close to being able to do that. We fixed a ton of stuff in bitmanip.md, > but I don't think there's been a thorough review of the port to find > other instances of TYPE_UNKNOWN INSNs. > > > The other thing if this code probably wants to handle GHOST type > instructions. While GHOST is used for instructions which generate no > code, it might seem they should return "more" as those INSNs take no > resources. But GHOST is actually used for things like the blockage insn > which should end a cycle from an issue standpoint. So the right > handling of a GHOST is something like this: > > if (type == TYPE_GHOST) > return 0; So there wasn't ever any follow-up. Given this was something Ventana was also carrying locally (with very minor differences) I went ahead and merged up the implementations and pushed the final result to the trunk. Attached is the patch that was actually committed. Jeff [-- Attachment #2: P --] [-- Type: text/plain, Size: 2038 bytes --] commit f088b768d01ae42385697584a2bcac141685dce2 Author: Jin Ma <jinma@linux.alibaba.com> Date: Wed Aug 9 13:52:06 2023 -0600 RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. Reference: https://github.com/gcc-mirror/gcc/commit/d0bc0cb66bcb0e6a5a5a31a9e900e8ccc98e34e5 RISC-V should also be implemented to handle no_insn patterns for pipelining. gcc/ChangeLog: * config/riscv/riscv.cc (riscv_sched_variable_issue): New function. (TARGET_SCHED_VARIABLE_ISSUE): New macro. Co-authored-by: Philipp Tomsich <philipp.tomsich@vrull.eu> Co-authored-by: Jeff Law <jlaw@ventanamicro.com> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index 7f2041a54ba..dfb519ab9a8 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -6698,6 +6698,31 @@ riscv_issue_rate (void) return tune_param->issue_rate; } +/* Implement TARGET_SCHED_VARIABLE_ISSUE. */ +static int +riscv_sched_variable_issue (FILE *, int, rtx_insn *insn, int more) +{ + if (DEBUG_INSN_P (insn)) + return more; + + rtx_code code = GET_CODE (PATTERN (insn)); + if (code == USE || code == CLOBBER) + return more; + + /* GHOST insns are used for blockage and similar cases which + effectively end a cycle. */ + if (get_attr_type (insn) == TYPE_GHOST) + return 0; + +#if 0 + /* If we ever encounter an insn with an unknown type, trip + an assert so we can find and fix this problem. */ + gcc_assert (get_attr_type (insn) != TYPE_UNKNOWN); +#endif + + return more - 1; +} + /* Auxiliary function to emit RISC-V ELF attribute. */ static void riscv_emit_attribute () @@ -8420,6 +8445,9 @@ riscv_frame_pointer_required (void) #undef TARGET_SCHED_ISSUE_RATE #define TARGET_SCHED_ISSUE_RATE riscv_issue_rate +#undef TARGET_SCHED_VARIABLE_ISSUE +#define TARGET_SCHED_VARIABLE_ISSUE riscv_sched_variable_issue + #undef TARGET_FUNCTION_OK_FOR_SIBCALL #define TARGET_FUNCTION_OK_FOR_SIBCALL riscv_function_ok_for_sibcall ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-08-09 19:56 ` Jeff Law @ 2023-08-11 2:12 ` Jin Ma 2023-08-11 2:30 ` Palmer Dabbelt 2023-08-11 13:22 ` Jeff Law 0 siblings, 2 replies; 11+ messages in thread From: Jin Ma @ 2023-08-11 2:12 UTC (permalink / raw) To: gcc-patches, Jeff Law Cc: richard.sandiford, kito.cheng, christoph.muellner, jinma.contrib > On 5/29/23 06:46, Jeff Law wrote: > > > > > > On 5/29/23 05:01, Jin Ma wrote: > >> Reference: > >> https://github.com/gcc-mirror/gcc/commit/d0bc0cb66bcb0e6a5a5a31a9e900e8ccc98e34e5 > >> > >> RISC-V should also be implemented to handle no_insn patterns for > >> pipelining. > >> > >> gcc/ChangeLog: > >> > >> * config/riscv/riscv.cc (riscv_sched_variable_issue): New function. > >> (TARGET_SCHED_VARIABLE_ISSUE): New macro. > >> --- > >> gcc/config/riscv/riscv.cc | 21 +++++++++++++++++++++ > >> 1 file changed, 21 insertions(+) > >> > >> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc > >> index 3954fc07a8b..559fa9cd7e0 100644 > >> --- a/gcc/config/riscv/riscv.cc > >> +++ b/gcc/config/riscv/riscv.cc > >> @@ -6225,6 +6225,24 @@ riscv_issue_rate (void) > >> return tune_param->issue_rate; > >> } > >> +/* Implement TARGET_SCHED_VARIABLE_ISSUE. */ > >> + > >> +static int > >> +riscv_sched_variable_issue (FILE *, int, rtx_insn *insn, int more) > >> +{ > >> + if (DEBUG_INSN_P (insn)) > >> + return more; > >> + > >> + rtx_code code = GET_CODE (PATTERN (insn)); > >> + if (code == USE || code == CLOBBER) > >> + return more; > >> + > >> + if (get_attr_type (insn) == TYPE_UNKNOWN) > >> + return more; > >> + > >> + return more - 1; > >> +} > > The problem is that INSN is *much* more likely to be a real instruction > > that takes real resources, even if it is TYPE_UNKNOWN. > > TYPE_UNKNOWN here is actually an indicator of what I would consider a > > bug in the backend, specifically that we have INSNs that do not provide > > a mapping for the schedulers to suitable types. > > > > With that in mind I'd much rather get to the point where we can do > > something like this for TYPE_UNKNOWN: > > > > type = get_attr_type (insn); > > gcc_assert (type != TYPE_UNKNOWN); > > > > That way if we ever encounter a TYPE_UNKNOWN during development, we can > > fix it in the md files in a sensible manner. I don't know if we are > > close to being able to do that. We fixed a ton of stuff in bitmanip.md, > > but I don't think there's been a thorough review of the port to find > > other instances of TYPE_UNKNOWN INSNs. > > > > > > The other thing if this code probably wants to handle GHOST type > > instructions. While GHOST is used for instructions which generate no > > code, it might seem they should return "more" as those INSNs take no > > resources. But GHOST is actually used for things like the blockage insn > > which should end a cycle from an issue standpoint. So the right > > handling of a GHOST is something like this: > > > > if (type == TYPE_GHOST) > > return 0; > So there wasn't ever any follow-up. Given this was something Ventana > was also carrying locally (with very minor differences) I went ahead and > merged up the implementations and pushed the final result to the trunk. > > > Attached is the patch that was actually committed. > > Jeff My fault, I'm very sorry for not replying to the patch follow-up, I just forgot this :) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-08-11 2:12 ` Jin Ma @ 2023-08-11 2:30 ` Palmer Dabbelt 2023-08-11 3:19 ` Jeff Law 2023-08-11 13:22 ` Jeff Law 1 sibling, 1 reply; 11+ messages in thread From: Palmer Dabbelt @ 2023-08-11 2:30 UTC (permalink / raw) To: gcc-patches Cc: gcc-patches, jeffreyalaw, richard.sandiford, Kito Cheng, christoph.muellner, jinma.contrib On Thu, 10 Aug 2023 19:12:06 PDT (-0700), gcc-patches@gcc.gnu.org wrote: >> On 5/29/23 06:46, Jeff Law wrote: >> > >> > >> > On 5/29/23 05:01, Jin Ma wrote: >> >> Reference: >> >> https://github.com/gcc-mirror/gcc/commit/d0bc0cb66bcb0e6a5a5a31a9e900e8ccc98e34e5 >> >> >> >> RISC-V should also be implemented to handle no_insn patterns for >> >> pipelining. >> >> >> >> gcc/ChangeLog: >> >> >> >> * config/riscv/riscv.cc (riscv_sched_variable_issue): New function. >> >> (TARGET_SCHED_VARIABLE_ISSUE): New macro. >> >> --- >> >> gcc/config/riscv/riscv.cc | 21 +++++++++++++++++++++ >> >> 1 file changed, 21 insertions(+) >> >> >> >> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc >> >> index 3954fc07a8b..559fa9cd7e0 100644 >> >> --- a/gcc/config/riscv/riscv.cc >> >> +++ b/gcc/config/riscv/riscv.cc >> >> @@ -6225,6 +6225,24 @@ riscv_issue_rate (void) >> >> return tune_param->issue_rate; >> >> } >> >> +/* Implement TARGET_SCHED_VARIABLE_ISSUE. */ >> >> + >> >> +static int >> >> +riscv_sched_variable_issue (FILE *, int, rtx_insn *insn, int more) >> >> +{ >> >> + if (DEBUG_INSN_P (insn)) >> >> + return more; >> >> + >> >> + rtx_code code = GET_CODE (PATTERN (insn)); >> >> + if (code == USE || code == CLOBBER) >> >> + return more; >> >> + >> >> + if (get_attr_type (insn) == TYPE_UNKNOWN) >> >> + return more; >> >> + >> >> + return more - 1; >> >> +} >> > The problem is that INSN is *much* more likely to be a real instruction >> > that takes real resources, even if it is TYPE_UNKNOWN. >> > TYPE_UNKNOWN here is actually an indicator of what I would consider a >> > bug in the backend, specifically that we have INSNs that do not provide >> > a mapping for the schedulers to suitable types. >> > >> > With that in mind I'd much rather get to the point where we can do >> > something like this for TYPE_UNKNOWN: >> > >> > type = get_attr_type (insn); >> > gcc_assert (type != TYPE_UNKNOWN); >> > >> > That way if we ever encounter a TYPE_UNKNOWN during development, we can >> > fix it in the md files in a sensible manner. I don't know if we are >> > close to being able to do that. We fixed a ton of stuff in bitmanip.md, >> > but I don't think there's been a thorough review of the port to find >> > other instances of TYPE_UNKNOWN INSNs. Sorry for being lost here, but I'm not sure where TYPE_UNKNOWN comes from. There's not a whole lot of instances in the code, and they all seem to be doing something very special. Is it just something we didn't do a '(set_attr "type" ...)' on? In that case it seems reasonable to have a dev-mode early failure: we've got some odd types now (like just the broad "bitmanip" one), but those can be split later. At least having some classification seems like the way to go, it's just an internal interface so we can make it better later. That said, it also smells like this is something that should be more generic than backend code? >> > The other thing if this code probably wants to handle GHOST type >> > instructions. While GHOST is used for instructions which generate no >> > code, it might seem they should return "more" as those INSNs take no >> > resources. But GHOST is actually used for things like the blockage insn >> > which should end a cycle from an issue standpoint. So the right >> > handling of a GHOST is something like this: >> > >> > if (type == TYPE_GHOST) >> > return 0; Lost again, here, there's almost no references to TYPE_GHOST (aside from a MIPS-ism that looks to have ended up in Loongarch). >> So there wasn't ever any follow-up. Given this was something Ventana >> was also carrying locally (with very minor differences) I went ahead and >> merged up the implementations and pushed the final result to the trunk. >> >> >> Attached is the patch that was actually committed. >> >> Jeff > > My fault, I'm very sorry for not replying to the patch follow-up, I just > forgot this :) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-08-11 2:30 ` Palmer Dabbelt @ 2023-08-11 3:19 ` Jeff Law 2023-08-11 3:45 ` Palmer Dabbelt 0 siblings, 1 reply; 11+ messages in thread From: Jeff Law @ 2023-08-11 3:19 UTC (permalink / raw) To: Palmer Dabbelt, gcc-patches Cc: richard.sandiford, Kito Cheng, christoph.muellner, jinma.contrib On 8/10/23 20:30, Palmer Dabbelt wrote: > > Sorry for being lost here, but I'm not sure where TYPE_UNKNOWN comes > from. There's not a whole lot of instances in the code, and they all > seem to be doing something very special. Is it just something we didn't > do a '(set_attr "type" ...)' on? Yup. TYPE_UNKNOWN means we don't have a type associated with the insn. As I've mentioned before this isn't a major problem if there's one or two here and there. But if most are TYPE_UNKNOWN, the the scheduler is going to do highly unnatural things. > > In that case it seems reasonable to have a dev-mode early failure: we've > got some odd types now (like just the broad "bitmanip" one), but those > can be split later. At least having some classification seems like the > way to go, it's just an internal interface so we can make it better later. > > That said, it also smells like this is something that should be more > generic than backend code? No, it's really a target issue. And what I was suggesting is that we get to the point where we can enable the currently #if 0'd assert so that if we introduce insns without an associated type, we get a nice early warning. I wasn't up for tackling that this week ;-) > >>> > The other thing if this code probably wants to handle GHOST type > >>> instructions. While GHOST is used for instructions which generate no >>> > code, it might seem they should return "more" as those INSNs take >>> no > resources. But GHOST is actually used for things like the >>> blockage insn > which should end a cycle from an issue standpoint. >>> So the right > handling of a GHOST is something like this: >>> > > if (type == TYPE_GHOST) >>> > return 0; > > Lost again, here, there's almost no references to TYPE_GHOST (aside from > a MIPS-ism that looks to have ended up in Loongarch). Search for "ghost" in riscv.md ;-) Jeff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-08-11 3:19 ` Jeff Law @ 2023-08-11 3:45 ` Palmer Dabbelt 2023-08-11 13:29 ` Jeff Law 0 siblings, 1 reply; 11+ messages in thread From: Palmer Dabbelt @ 2023-08-11 3:45 UTC (permalink / raw) To: jeffreyalaw Cc: gcc-patches, richard.sandiford, Kito Cheng, christoph.muellner, jinma.contrib On Thu, 10 Aug 2023 20:19:02 PDT (-0700), jeffreyalaw@gmail.com wrote: > > > On 8/10/23 20:30, Palmer Dabbelt wrote: > >> >> Sorry for being lost here, but I'm not sure where TYPE_UNKNOWN comes >> from. There's not a whole lot of instances in the code, and they all >> seem to be doing something very special. Is it just something we didn't >> do a '(set_attr "type" ...)' on? > Yup. TYPE_UNKNOWN means we don't have a type associated with the insn. > As I've mentioned before this isn't a major problem if there's one or > two here and there. But if most are TYPE_UNKNOWN, the the scheduler is > going to do highly unnatural things. OK, that seems like the way to go. I still think it's likely we'll need to split up these types more, but that's something we can only deal with when there's HW that behaves oddly. >> In that case it seems reasonable to have a dev-mode early failure: we've >> got some odd types now (like just the broad "bitmanip" one), but those >> can be split later. At least having some classification seems like the >> way to go, it's just an internal interface so we can make it better later. >> >> That said, it also smells like this is something that should be more >> generic than backend code? > No, it's really a target issue. And what I was suggesting is that we > get to the point where we can enable the currently #if 0'd assert so > that if we introduce insns without an associated type, we get a nice > early warning. I wasn't up for tackling that this week ;-) I was thinking of some sort of "TARGET_ALLOWS_UNKNOWN_INSNS" hook, but poking around the uses that might not be meaningfully simpler than just rejecting these in the backend -- certainly simpler if we're just worried about RISC-V ;) This seems pretty mechinacial: just scrub through our MDs to check for any un-typed insns, then add the assert and fix the failures. You're more than welcome to have at it, but LMK if you want me to try and find some time for someone to do it -- certainly seems like a good way for someone new to dig in a bit. >>>> > The other thing if this code probably wants to handle GHOST type > >>>> instructions. While GHOST is used for instructions which generate no >>>> > code, it might seem they should return "more" as those INSNs take >>>> no > resources. But GHOST is actually used for things like the >>>> blockage insn > which should end a cycle from an issue standpoint. >>>> So the right > handling of a GHOST is something like this: >>>> > > if (type == TYPE_GHOST) >>>> > return 0; >> >> Lost again, here, there's almost no references to TYPE_GHOST (aside from >> a MIPS-ism that looks to have ended up in Loongarch). > Search for "ghost" in riscv.md ;-) Thanks, the "return 0" makes sense. > > Jeff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-08-11 3:45 ` Palmer Dabbelt @ 2023-08-11 13:29 ` Jeff Law 2023-08-14 20:33 ` Edwin Lu 0 siblings, 1 reply; 11+ messages in thread From: Jeff Law @ 2023-08-11 13:29 UTC (permalink / raw) To: Palmer Dabbelt Cc: gcc-patches, richard.sandiford, Kito Cheng, christoph.muellner, jinma.contrib On 8/10/23 21:45, Palmer Dabbelt wrote: > > OK, that seems like the way to go. I still think it's likely we'll need > to split up these types more, but that's something we can only deal with > when there's HW that behaves oddly. Yea, but I think we can fault this in as problematic hardware arrives. >> No, it's really a target issue. And what I was suggesting is that we >> get to the point where we can enable the currently #if 0'd assert so >> that if we introduce insns without an associated type, we get a nice >> early warning. I wasn't up for tackling that this week ;-) > > I was thinking of some sort of "TARGET_ALLOWS_UNKNOWN_INSNS" hook, but > poking around the uses that might not be meaningfully simpler than just > rejecting these in the backend -- certainly simpler if we're just > worried about RISC-V ;) Not all ports have types at all. Some use types for things other than scheduling. It'd be a huge can of worms. > > This seems pretty mechinacial: just scrub through our MDs to check for > any un-typed insns, then add the assert and fix the failures. You're > more than welcome to have at it, but LMK if you want me to try and find > some time for someone to do it -- certainly seems like a good way for > someone new to dig in a bit. Yes, definitely mechanical. And yes, it's a good way for someone to start to get familiar with these bits -- I used the lack of types on some of the bitmanip insns to help ramp up Raphael and one of the RAU guys in this space. Jeff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-08-11 13:29 ` Jeff Law @ 2023-08-14 20:33 ` Edwin Lu 2023-08-14 22:06 ` Jeff Law 0 siblings, 1 reply; 11+ messages in thread From: Edwin Lu @ 2023-08-14 20:33 UTC (permalink / raw) To: Jeff Law, Palmer Dabbelt Cc: gcc-patches, richard.sandiford, Kito Cheng, christoph.muellner, jinma.contrib On 8/11/2023 6:29 AM, Jeff Law via Gcc-patches wrote: > > > On 8/10/23 21:45, Palmer Dabbelt wrote: > >> >> This seems pretty mechinacial: just scrub through our MDs to check >> for any un-typed insns, then add the assert and fix the failures. >> You're more than welcome to have at it, but LMK if you want me to try >> and find some time for someone to do it -- certainly seems like a >> good way for someone new to dig in a bit. > Yes, definitely mechanical. And yes, it's a good way for someone to > start to get familiar with these bits -- I used the lack of types on > some of the bitmanip insns to help ramp up Raphael and one of the RAU > guys in this space. > > Jeff Hi, Palmer sent me this thread to take a look at. I can start working on this. Edwin Lu ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-08-14 20:33 ` Edwin Lu @ 2023-08-14 22:06 ` Jeff Law 0 siblings, 0 replies; 11+ messages in thread From: Jeff Law @ 2023-08-14 22:06 UTC (permalink / raw) To: Edwin Lu, Palmer Dabbelt Cc: gcc-patches, richard.sandiford, Kito Cheng, christoph.muellner, jinma.contrib On 8/14/23 14:33, Edwin Lu wrote: > > On 8/11/2023 6:29 AM, Jeff Law via Gcc-patches wrote: >> >> >> On 8/10/23 21:45, Palmer Dabbelt wrote: >> >>> >>> This seems pretty mechinacial: just scrub through our MDs to check >>> for any un-typed insns, then add the assert and fix the failures. >>> You're more than welcome to have at it, but LMK if you want me to try >>> and find some time for someone to do it -- certainly seems like a >>> good way for someone new to dig in a bit. >> Yes, definitely mechanical. And yes, it's a good way for someone to >> start to get familiar with these bits -- I used the lack of types on >> some of the bitmanip insns to help ramp up Raphael and one of the RAU >> guys in this space. >> >> Jeff > > Hi, Palmer sent me this thread to take a look at. I can start working on > this. Sounds good. The goal is to make sure that every insn has a type and once we hit that milestone we can enable the currently #if 0'd assert in riscv_sched_variable_issue to help ensure we don't introduce any new insns without types in the future. If you have any questions, don't hesitate to reach out. jeff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE. 2023-08-11 2:12 ` Jin Ma 2023-08-11 2:30 ` Palmer Dabbelt @ 2023-08-11 13:22 ` Jeff Law 1 sibling, 0 replies; 11+ messages in thread From: Jeff Law @ 2023-08-11 13:22 UTC (permalink / raw) To: Jin Ma, gcc-patches Cc: richard.sandiford, kito.cheng, christoph.muellner, jinma.contrib On 8/10/23 20:12, Jin Ma wrote: > > My fault, I'm very sorry for not replying to the patch follow-up, I just > forgot this :) No worries. We're tracking it in patchwork and it also overlaps with some work we had internally at Ventana. So it was trivial to pick it up once it was clear it'd fallen through the cracks. jeff ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-08-14 22:06 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-29 11:01 [PATCH] RISC-V: Handle no_insn in TARGET_SCHED_VARIABLE_ISSUE Jin Ma 2023-05-29 12:46 ` Jeff Law 2023-08-09 19:56 ` Jeff Law 2023-08-11 2:12 ` Jin Ma 2023-08-11 2:30 ` Palmer Dabbelt 2023-08-11 3:19 ` Jeff Law 2023-08-11 3:45 ` Palmer Dabbelt 2023-08-11 13:29 ` Jeff Law 2023-08-14 20:33 ` Edwin Lu 2023-08-14 22:06 ` Jeff Law 2023-08-11 13:22 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).