diff mbox series

[04/44] RISC-V: Sanitise NEED_EQ_NE_P case with `riscv_emit_int_compare'

Message ID alpine.DEB.2.20.2311171401210.5892@tpp.orcam.me.uk
State Committed
Commit 9f5aa4e210cbcb621ead82f4b56482deaa548c13
Delegated to: Kito Cheng
Headers
Series RISC-V: Various if-conversion fixes and improvements |

Checks

Context Check Description
rivoscibot/toolchain-ci-rivos-lint success Lint passed
rivoscibot/toolchain-ci-rivos-apply-patch success Patch applied
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gc-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gc_zba_zbb_zbc_zbs-lp64d-non-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv32gc_zba_zbb_zbc_zbs-ilp32d-non-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--newlib-rv64gcv-lp64d-multilib success Build passed
rivoscibot/toolchain-ci-rivos-build--linux-rv64gcv-lp64d-multilib success Build passed

Commit Message

Maciej W. Rozycki Nov. 19, 2023, 5:36 a.m. UTC 
  For the NEED_EQ_NE_P `riscv_emit_int_compare' is documented to only emit 
EQ or NE comparisons against zero, however it does not catch incorrect 
use where a non-equality comparison has been requested and falls through 
to the general case then.  Add a safety guard to catch such a case then.

Arguably the NEED_EQ_NE_P case would best be moved into a function of 
its own, but let's leave it for a separate cleanup.

	gcc/
	* config/riscv/riscv.cc (riscv_emit_int_compare): Bail out if
	NEED_EQ_NE_P but the comparison is neither EQ nor NE.	
---
FWIW the structure of code here clearly shows the NEED_EQ_NE_P case has 
been bolted on as an afterthought rather than how this piece would look 
if written from scratch right away.  Let's defer any further cleanups at 
this stage of the development cycle though.
---
 gcc/config/riscv/riscv.cc |    1 +
 1 file changed, 1 insertion(+)

gcc-riscv-emit-int-compare-need-eq-ne.diff

Comments

Kito Cheng Nov. 19, 2023, 5:53 a.m. UTC | #1 
LGTM

On Sun, Nov 19, 2023 at 1:36 PM Maciej W. Rozycki <macro@embecosm.com> wrote:
>
> For the NEED_EQ_NE_P `riscv_emit_int_compare' is documented to only emit
> EQ or NE comparisons against zero, however it does not catch incorrect
> use where a non-equality comparison has been requested and falls through
> to the general case then.  Add a safety guard to catch such a case then.
>
> Arguably the NEED_EQ_NE_P case would best be moved into a function of
> its own, but let's leave it for a separate cleanup.
>
>         gcc/
>         * config/riscv/riscv.cc (riscv_emit_int_compare): Bail out if
>         NEED_EQ_NE_P but the comparison is neither EQ nor NE.
> ---
> FWIW the structure of code here clearly shows the NEED_EQ_NE_P case has
> been bolted on as an afterthought rather than how this piece would look
> if written from scratch right away.  Let's defer any further cleanups at
> this stage of the development cycle though.
> ---
>  gcc/config/riscv/riscv.cc |    1 +
>  1 file changed, 1 insertion(+)
>
> gcc-riscv-emit-int-compare-need-eq-ne.diff
> Index: gcc/gcc/config/riscv/riscv.cc
> ===================================================================
> --- gcc.orig/gcc/config/riscv/riscv.cc
> +++ gcc/gcc/config/riscv/riscv.cc
> @@ -3779,6 +3779,7 @@ riscv_emit_int_compare (enum rtx_code *c
>           *op1 = const0_rtx;
>           return;
>         }
> +      gcc_unreachable ();
>      }
>
>    if (splittable_const_int_operand (*op1, VOIDmode))
diff mbox series

Patch

Index: gcc/gcc/config/riscv/riscv.cc
===================================================================
--- gcc.orig/gcc/config/riscv/riscv.cc
+++ gcc/gcc/config/riscv/riscv.cc
@@ -3779,6 +3779,7 @@  riscv_emit_int_compare (enum rtx_code *c
 	  *op1 = const0_rtx;
 	  return;
 	}
+      gcc_unreachable ();
     }
 
   if (splittable_const_int_operand (*op1, VOIDmode))