diff mbox series

[RFC] Make genautomata.cc output reflect insn-attr.h expectation:

Message ID 20231031225144.24448-1-ewlu@rivosinc.com
State Accepted
Headers
Series [RFC] Make genautomata.cc output reflect insn-attr.h expectation: |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Edwin Lu Oct. 31, 2023, 10:51 p.m. UTC 
  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(-)

Comments

Vladimir Makarov Nov. 1, 2023, 3:07 p.m. UTC | #1 
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 ();
Edwin Lu Nov. 1, 2023, 5:24 p.m. UTC | #2 
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 mbox series

Patch

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 ();