| Message ID | 20231031225144.24448-1-ewlu@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] Make genautomata.cc output reflect insn-attr.h expectation: | expand |
On 10/31/23 18:51, Edwin Lu wrote: > genattr.cc currently generates insn-attr.h with the following structure: > > #if CPU_UNITS_QUERY > extern int get_cpu_unit_code (const char *); > extern int cpu_unit_reservation_p (state_t, int); > #endif > extern bool insn_has_dfa_reservation_p (rtx_insn *); > > however genautomata.cc generates insn-automata.cc with the following structure: > #if CPU_UNITS_QUERY > int get_cpu_unit_code (const char * ) { ... } > int cpu_unit_reservation_p (state_t, int) { ... } > bool insn_has_dfa_reservation_p (rtx_insn *) { ... } > #endif > > I'm not sure if insn_has_dfa_reservation_p is supposed to be a part of the > CPU_UNITS_QUERY conditional group or not. For consistency, I would like to > move it outside of the group. No, it should be not considered a part of cpu unit query group. The function just says that there is any cpu reservation by insns. Two other functions say that the state is still reserving a particular cpu unit. Using these 2 functions requires a lot of memory for their implementation and prevent further dfa minimizations. The functions should be used mostly for VLIW CPUs when we need this information to generate machine insns (e.g, ia64 VLIW insn template). > This would move insn_has_dfa_reservation_p out of the #if CPU_UNITS_QUERY > conditional inside of insn-automata.cc. This would allow us to see if the > scheduler is trying to schedule an insn with a type which is not associated > with a cpu unit or insn reservation through the TARGET_SCHED_VARIABLE_ISSUE > hook. > > If there is a reason for insn_has_dfa_reservation_p being within the > conditional, please let me know! It seems a typo. The patch is ok for me. Thank you for finding this out. > gcc/Changelog: > > * genautomata.cc (write_automata): move endif > > Signed-off-by: Edwin Lu <ewlu@rivosinc.com> > --- > gcc/genautomata.cc | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/genautomata.cc b/gcc/genautomata.cc > index 72f01686d6b..9dda25e5ba2 100644 > --- a/gcc/genautomata.cc > +++ b/gcc/genautomata.cc > @@ -9503,9 +9503,9 @@ write_automata (void) > fprintf (output_file, "\n#if %s\n\n", CPU_UNITS_QUERY_MACRO_NAME); > output_get_cpu_unit_code_func (); > output_cpu_unit_reservation_p (); > - output_insn_has_dfa_reservation_p (); > fprintf (output_file, "\n#endif /* #if %s */\n\n", > CPU_UNITS_QUERY_MACRO_NAME); > + output_insn_has_dfa_reservation_p (); > output_dfa_clean_insn_cache_func (); > output_dfa_start_func (); > output_dfa_finish_func ();
On 11/1/2023 8:07 AM, Vladimir Makarov wrote: > > On 10/31/23 18:51, Edwin Lu wrote: >> genattr.cc currently generates insn-attr.h with the following structure: >> >> #if CPU_UNITS_QUERY >> extern int get_cpu_unit_code (const char *); >> extern int cpu_unit_reservation_p (state_t, int); >> #endif >> extern bool insn_has_dfa_reservation_p (rtx_insn *); >> >> however genautomata.cc generates insn-automata.cc with the following >> structure: >> #if CPU_UNITS_QUERY >> int get_cpu_unit_code (const char * ) { ... } >> int cpu_unit_reservation_p (state_t, int) { ... } >> bool insn_has_dfa_reservation_p (rtx_insn *) { ... } >> #endif >> >> I'm not sure if insn_has_dfa_reservation_p is supposed to be a part of >> the >> CPU_UNITS_QUERY conditional group or not. For consistency, I would >> like to >> move it outside of the group. > > No, it should be not considered a part of cpu unit query group. The > function just says that there is any cpu reservation by insns. > > Two other functions say that the state is still reserving a particular > cpu unit. Using these 2 functions requires a lot of memory for their > implementation and prevent further dfa minimizations. The functions > should be used mostly for VLIW CPUs when we need this information to > generate machine insns (e.g, ia64 VLIW insn template). > Thanks for the clarification! >> This would move insn_has_dfa_reservation_p out of the #if CPU_UNITS_QUERY >> conditional inside of insn-automata.cc. This would allow us to see if the >> scheduler is trying to schedule an insn with a type which is not >> associated >> with a cpu unit or insn reservation through the >> TARGET_SCHED_VARIABLE_ISSUE >> hook. >> >> If there is a reason for insn_has_dfa_reservation_p being within the >> conditional, please let me know! > > It seems a typo. > > The patch is ok for me. Thank you for finding this out. > Committed! Edwin >> gcc/Changelog: >> >> * genautomata.cc (write_automata): move endif >> >> Signed-off-by: Edwin Lu <ewlu@rivosinc.com> >> --- >> gcc/genautomata.cc | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/gcc/genautomata.cc b/gcc/genautomata.cc >> index 72f01686d6b..9dda25e5ba2 100644 >> --- a/gcc/genautomata.cc >> +++ b/gcc/genautomata.cc >> @@ -9503,9 +9503,9 @@ write_automata (void) >> fprintf (output_file, "\n#if %s\n\n", CPU_UNITS_QUERY_MACRO_NAME); >> output_get_cpu_unit_code_func (); >> output_cpu_unit_reservation_p (); >> - output_insn_has_dfa_reservation_p (); >> fprintf (output_file, "\n#endif /* #if %s */\n\n", >> CPU_UNITS_QUERY_MACRO_NAME); >> + output_insn_has_dfa_reservation_p (); >> output_dfa_clean_insn_cache_func (); >> output_dfa_start_func (); >> output_dfa_finish_func (); > >
diff --git a/gcc/genautomata.cc b/gcc/genautomata.cc index 72f01686d6b..9dda25e5ba2 100644 --- a/gcc/genautomata.cc +++ b/gcc/genautomata.cc @@ -9503,9 +9503,9 @@ write_automata (void) fprintf (output_file, "\n#if %s\n\n", CPU_UNITS_QUERY_MACRO_NAME); output_get_cpu_unit_code_func (); output_cpu_unit_reservation_p (); - output_insn_has_dfa_reservation_p (); fprintf (output_file, "\n#endif /* #if %s */\n\n", CPU_UNITS_QUERY_MACRO_NAME); + output_insn_has_dfa_reservation_p (); output_dfa_clean_insn_cache_func (); output_dfa_start_func (); output_dfa_finish_func ();
genattr.cc currently generates insn-attr.h with the following structure: #if CPU_UNITS_QUERY extern int get_cpu_unit_code (const char *); extern int cpu_unit_reservation_p (state_t, int); #endif extern bool insn_has_dfa_reservation_p (rtx_insn *); however genautomata.cc generates insn-automata.cc with the following structure: #if CPU_UNITS_QUERY int get_cpu_unit_code (const char * ) { ... } int cpu_unit_reservation_p (state_t, int) { ... } bool insn_has_dfa_reservation_p (rtx_insn *) { ... } #endif I'm not sure if insn_has_dfa_reservation_p is supposed to be a part of the CPU_UNITS_QUERY conditional group or not. For consistency, I would like to move it outside of the group. This would move insn_has_dfa_reservation_p out of the #if CPU_UNITS_QUERY conditional inside of insn-automata.cc. This would allow us to see if the scheduler is trying to schedule an insn with a type which is not associated with a cpu unit or insn reservation through the TARGET_SCHED_VARIABLE_ISSUE hook. If there is a reason for insn_has_dfa_reservation_p being within the conditional, please let me know! gcc/Changelog: * genautomata.cc (write_automata): move endif Signed-off-by: Edwin Lu <ewlu@rivosinc.com> --- gcc/genautomata.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)