diff mbox series

[V3,4/4] RISC-V: Enable assert for insn_has_dfa_reservation

Message ID 20240112180844.2005246-5-ewlu@rivosinc.com
State Unresolved
Headers
Series RISC-V: Associate typed insns to dfa reservation |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Edwin Lu Jan. 12, 2024, 6:08 p.m. UTC 
  Enables assert that every typed instruction is associated with a
dfa reservation

gcc/ChangeLog:

	* config/riscv/riscv.cc (riscv_sched_variable_issue): enable assert

Signed-off-by: Edwin Lu <ewlu@rivosinc.com>
---
V2:
- No changes
V3:
- No changes
---
 gcc/config/riscv/riscv.cc | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Robin Dapp Jan. 25, 2024, 5:06 p.m. UTC | #1 
>    /* If we ever encounter an insn without an insn reservation, trip
>       an assert so we can find and fix this problem.  */
> -#if 0
> +  if (! insn_has_dfa_reservation_p (insn)) {
> +    print_rtl(stderr, insn);
> +    fprintf(stderr, "%d", get_attr_type (insn));
> +  }
>    gcc_assert (insn_has_dfa_reservation_p (insn));
> -#endif
>  
>    return more - 1;
>  }

I was thinking about make the gcc_assert a gcc_checking_assert so,
in case we accidentally forget something at any point, it would
only gracefully degrade in a release build.  As we already have
a hard assert for the type the patch (and not many test with
enable checking anyway) this is OK IMHO.

I suppose you tested with all available -mtune options?

Regards
 Robin
Edwin Lu Jan. 26, 2024, 6:37 p.m. UTC | #2 
On 1/25/2024 9:06 AM, Robin Dapp wrote:
>>     /* If we ever encounter an insn without an insn reservation, trip
>>        an assert so we can find and fix this problem.  */
>> -#if 0
>> +  if (! insn_has_dfa_reservation_p (insn)) {
>> +    print_rtl(stderr, insn);
>> +    fprintf(stderr, "%d", get_attr_type (insn));
>> +  }
>>     gcc_assert (insn_has_dfa_reservation_p (insn));
>> -#endif
>>   
>>     return more - 1;
>>   }

Oops accidentally left my debugging statements in the patch.

> 
> I was thinking about make the gcc_assert a gcc_checking_assert so,
> in case we accidentally forget something at any point, it would
> only gracefully degrade in a release build.  As we already have
> a hard assert for the type the patch (and not many test with
> enable checking anyway) this is OK IMHO.
> 
> I suppose you tested with all available -mtune options?


I ran the testsuite on all three tunes using linux rv64gcv. generic-ooo 
had some bugs fixed while rocket and sifive-7-series had around 37 new 
scan dump failures which I think is to be expected. No ICE's from the 
hard gcc_assert on any of the tunes so I think it should be fine as is.

Edwin
Edwin Lu Feb. 1, 2024, 1:42 a.m. UTC | #3 
On 1/25/2024 9:06 AM, Robin Dapp wrote:
>>     /* If we ever encounter an insn without an insn reservation, trip
>>        an assert so we can find and fix this problem.  */
>> -#if 0
>> +  if (! insn_has_dfa_reservation_p (insn)) {
>> +    print_rtl(stderr, insn);
>> +    fprintf(stderr, "%d", get_attr_type (insn));
>> +  }
>>     gcc_assert (insn_has_dfa_reservation_p (insn));
>> -#endif
>>   
>>     return more - 1;
>>   }
> 
> I was thinking about make the gcc_assert a gcc_checking_assert so,
> in case we accidentally forget something at any point, it would
> only gracefully degrade in a release build.  As we already have
> a hard assert for the type the patch (and not many test with
> enable checking anyway) this is OK IMHO.
> 
> I suppose you tested with all available -mtune options?
> 
> Regards
>   Robin
> 
> 

Committed without the debugging stuff!

Edwin
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index ee1a57b321d..c428d3e4e58 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -8215,9 +8215,11 @@  riscv_sched_variable_issue (FILE *, int, rtx_insn *insn, int more)
 
   /* If we ever encounter an insn without an insn reservation, trip
      an assert so we can find and fix this problem.  */
-#if 0
+  if (! insn_has_dfa_reservation_p (insn)) {
+    print_rtl(stderr, insn);
+    fprintf(stderr, "%d", get_attr_type (insn));
+  }
   gcc_assert (insn_has_dfa_reservation_p (insn));
-#endif
 
   return more - 1;
 }