[PATCH] phiopt: Fix ICE with large --param l1-cache-line-size= [PR112887]

[PATCH] phiopt: Fix ICE with large --param l1-cache-line-size= [PR112887]

[Jakub Jelinek]

Hi!

This function is never called when param_l1_cache_line_size is 0,
but it uses int and unsigned int variables to hold alignment in
bits, so for large param_l1_cache_line_size it is zero and e.g.
DECL_ALIGN () % param_align_bits can divide by zero.
Looking at the code, the function uses tree_fits_uhwi_p on the trees
before converting them using tree_to_uhwi to int variables, which
looks just wrong, either it would need to punt if it doesn't fit
into those and also check for overflows during the computation,
or use unsigned HOST_WIDE_INT for all of this.  That also fixes
the division by zero, as param_l1_cache_line_size maximum is INT_MAX,
that multiplied by 8 will always fit.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2023-12-09  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/112887
	* tree-ssa-phiopt.cc (hoist_adjacent_loads): Change type of
	param_align, param_align_bits, offset1, offset2, size2 and align1
	variables from int or unsigned int to unsigned HOST_WIDE_INT.

	* gcc.dg/pr112887.c: New test.

--- gcc/tree-ssa-phiopt.cc.jj	2023-11-14 10:52:16.195275972 +0100
+++ gcc/tree-ssa-phiopt.cc	2023-12-08 16:25:29.166747347 +0100
@@ -3757,8 +3757,8 @@ static void
 hoist_adjacent_loads (basic_block bb0, basic_block bb1,
 		      basic_block bb2, basic_block bb3)
 {
-  int param_align = param_l1_cache_line_size;
-  unsigned param_align_bits = (unsigned) (param_align * BITS_PER_UNIT);
+  unsigned HOST_WIDE_INT param_align = param_l1_cache_line_size;
+  unsigned HOST_WIDE_INT param_align_bits = param_align * BITS_PER_UNIT;
   gphi_iterator gsi;
 
   /* Walk the phis in bb3 looking for an opportunity.  We are looking
@@ -3770,8 +3770,7 @@ hoist_adjacent_loads (basic_block bb0, b
       gimple *def1, *def2;
       tree arg1, arg2, ref1, ref2, field1, field2;
       tree tree_offset1, tree_offset2, tree_size2, next;
-      int offset1, offset2, size2;
-      unsigned align1;
+      unsigned HOST_WIDE_INT offset1, offset2, size2, align1;
       gimple_stmt_iterator gsi2;
       basic_block bb_for_def1, bb_for_def2;
 
--- gcc/testsuite/gcc.dg/pr112887.c.jj	2023-12-08 16:31:30.708697160 +0100
+++ gcc/testsuite/gcc.dg/pr112887.c	2023-12-08 16:27:06.662385487 +0100
@@ -0,0 +1,13 @@
+/* PR tree-optimization/112887 */
+/* { dg-do compile } */
+/* { dg-options "-O2 --param=l1-cache-line-size=0x20000000" } */
+
+void bar (long);
+long c;
+struct S { long a, b; } s;
+
+void
+foo (void)
+{
+  bar (c ? s.a : s.b);
+}

	Jakub

Re: [PATCH] phiopt: Fix ICE with large --param l1-cache-line-size= [PR112887]

[Richard Biener]

> Am 09.12.2023 um 10:35 schrieb Jakub Jelinek <jakub@redhat.com>:
> 
> Hi!
> 
> This function is never called when param_l1_cache_line_size is 0,
> but it uses int and unsigned int variables to hold alignment in
> bits, so for large param_l1_cache_line_size it is zero and e.g.
> DECL_ALIGN () % param_align_bits can divide by zero.
> Looking at the code, the function uses tree_fits_uhwi_p on the trees
> before converting them using tree_to_uhwi to int variables, which
> looks just wrong, either it would need to punt if it doesn't fit
> into those and also check for overflows during the computation,
> or use unsigned HOST_WIDE_INT for all of this.  That also fixes
> the division by zero, as param_l1_cache_line_size maximum is INT_MAX,
> that multiplied by 8 will always fit.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok

> 2023-12-09  Jakub Jelinek  <jakub@redhat.com>
> 
>    PR tree-optimization/112887
>    * tree-ssa-phiopt.cc (hoist_adjacent_loads): Change type of
>    param_align, param_align_bits, offset1, offset2, size2 and align1
>    variables from int or unsigned int to unsigned HOST_WIDE_INT.
> 
>    * gcc.dg/pr112887.c: New test.
> 
> --- gcc/tree-ssa-phiopt.cc.jj    2023-11-14 10:52:16.195275972 +0100
> +++ gcc/tree-ssa-phiopt.cc    2023-12-08 16:25:29.166747347 +0100
> @@ -3757,8 +3757,8 @@ static void
> hoist_adjacent_loads (basic_block bb0, basic_block bb1,
>              basic_block bb2, basic_block bb3)
> {
> -  int param_align = param_l1_cache_line_size;
> -  unsigned param_align_bits = (unsigned) (param_align * BITS_PER_UNIT);
> +  unsigned HOST_WIDE_INT param_align = param_l1_cache_line_size;
> +  unsigned HOST_WIDE_INT param_align_bits = param_align * BITS_PER_UNIT;
>   gphi_iterator gsi;
> 
>   /* Walk the phis in bb3 looking for an opportunity.  We are looking
> @@ -3770,8 +3770,7 @@ hoist_adjacent_loads (basic_block bb0, b
>       gimple *def1, *def2;
>       tree arg1, arg2, ref1, ref2, field1, field2;
>       tree tree_offset1, tree_offset2, tree_size2, next;
> -      int offset1, offset2, size2;
> -      unsigned align1;
> +      unsigned HOST_WIDE_INT offset1, offset2, size2, align1;
>       gimple_stmt_iterator gsi2;
>       basic_block bb_for_def1, bb_for_def2;
> 
> --- gcc/testsuite/gcc.dg/pr112887.c.jj    2023-12-08 16:31:30.708697160 +0100
> +++ gcc/testsuite/gcc.dg/pr112887.c    2023-12-08 16:27:06.662385487 +0100
> @@ -0,0 +1,13 @@
> +/* PR tree-optimization/112887 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 --param=l1-cache-line-size=0x20000000" } */
> +
> +void bar (long);
> +long c;
> +struct S { long a, b; } s;
> +
> +void
> +foo (void)
> +{
> +  bar (c ? s.a : s.b);
> +}
> 
>    Jakub
>