Re: [PATCH] Fix internal error in GIMPLE DSE

2024-02-27 Thread Richard Biener
On Tue, Feb 27, 2024 at 1:50 PM Eric Botcazou  wrote:
>
> Hi,
>
> this is a regression present on the mainline, 13 and 12 branches.  For the
> attached Ada case, it's a tree checking failure on the mainline at -O:
>
> +===GNAT BUG DETECTED==+
> | 14.0.1 20240226 (experimental) [master r14-9171-g4972f97a265]  GCC error:|
> | tree check: expected tree that contains 'decl common' structure, |
> | have 'component_ref' in tree_could_trap_p, at tree-eh.cc:2733|
> | Error detected around /home/eric/cvs/gcc/gcc/testsuite/gnat.dg/opt104.adb:
>
> Time is a 10-byte record and Packed_Rec.T is placed at bit-offset 65 because
> of the packing. so tree-ssa-dse.cc:setup_live_bytes_from_ref has computed a
> const_size of 88 from ref->offset of 65 and ref->max_size of 80.
>
> Then in tree-ssa-dse.cc:compute_trims:
>
> 411   int last_live = bitmap_last_set_bit (live);
> (gdb) next
> 412   if (ref->size.is_constant (_size))
> (gdb)
> 414   int last_orig = (const_size / BITS_PER_UNIT) - 1;
> (gdb)
> 418   *trim_tail = last_orig - last_live;
>
> (gdb) call debug_bitmap (live)
> n_bits = 256, set = {0 1 2 3 4 5 6 7 8 9 10 }
> (gdb) p last_live
> $33 = 10
> (gdb) p const_size
> $34 = 80
> (gdb) p last_orig
> $35 = 9
> (gdb) p *trim_tail
> $36 = -1
>
> In other words, compute_trims is overlooking the alignment adjustments applied
> earlier by setup_live_bytes_from_ref.  Moveover it reads:
>
>   /* We use sbitmaps biased such that ref->offset is bit zero and the bitmap
>  extends through ref->size.  So we know that in the original bitmap
>  bits 0..ref->size were true.  We don't actually need the bitmap, just
>  the REF to compute the trims.  */
>
> But setup_live_bytes_from_ref used ref->max_size instead of ref->size.
>
> It appears that all the callers of compute_trims assume that ref->offset is
> byte aligned and that the trimmed bytes are relative to ref->size, so the
> patch simply adds an early return if either condition is not fulfilled
>
> Tested on x86-64/Linux, OK for all the affected branches?

OK.

Thanks,
Richard.

>
> 2024-02-27  Eric Botcazou  
>
> * tree-ssa-dse.cc (compute_trims): Fix description.  Return early
> if ref->offset is not byte aligned or ref->size is not known to be
> equal to ref->max_size.
> (maybe_trim_complex_store): Fix description.
> (maybe_trim_constructor_store): Likewise.
> (maybe_trim_partially_dead_store): Likewise.
>
>
> 2024-02-27  Eric Botcazou  
>
> * gnat.dg/opt104.ads, gnat.dg/opt104.adb! New test.
>
> --
> Eric Botcazou


[PATCH] Fix internal error in GIMPLE DSE

2024-02-27 Thread Eric Botcazou
Hi,

this is a regression present on the mainline, 13 and 12 branches.  For the 
attached Ada case, it's a tree checking failure on the mainline at -O:

+===GNAT BUG DETECTED==+
| 14.0.1 20240226 (experimental) [master r14-9171-g4972f97a265]  GCC error:|
| tree check: expected tree that contains 'decl common' structure, |
| have 'component_ref' in tree_could_trap_p, at tree-eh.cc:2733|
| Error detected around /home/eric/cvs/gcc/gcc/testsuite/gnat.dg/opt104.adb:

Time is a 10-byte record and Packed_Rec.T is placed at bit-offset 65 because 
of the packing. so tree-ssa-dse.cc:setup_live_bytes_from_ref has computed a 
const_size of 88 from ref->offset of 65 and ref->max_size of 80.

Then in tree-ssa-dse.cc:compute_trims:

411   int last_live = bitmap_last_set_bit (live);
(gdb) next
412   if (ref->size.is_constant (_size))
(gdb) 
414   int last_orig = (const_size / BITS_PER_UNIT) - 1;
(gdb) 
418   *trim_tail = last_orig - last_live;

(gdb) call debug_bitmap (live)
n_bits = 256, set = {0 1 2 3 4 5 6 7 8 9 10 }
(gdb) p last_live
$33 = 10
(gdb) p const_size
$34 = 80
(gdb) p last_orig
$35 = 9
(gdb) p *trim_tail
$36 = -1

In other words, compute_trims is overlooking the alignment adjustments applied 
earlier by setup_live_bytes_from_ref.  Moveover it reads:

  /* We use sbitmaps biased such that ref->offset is bit zero and the bitmap
 extends through ref->size.  So we know that in the original bitmap
 bits 0..ref->size were true.  We don't actually need the bitmap, just
 the REF to compute the trims.  */

But setup_live_bytes_from_ref used ref->max_size instead of ref->size.

It appears that all the callers of compute_trims assume that ref->offset is 
byte aligned and that the trimmed bytes are relative to ref->size, so the 
patch simply adds an early return if either condition is not fulfilled

Tested on x86-64/Linux, OK for all the affected branches?


2024-02-27  Eric Botcazou  

* tree-ssa-dse.cc (compute_trims): Fix description.  Return early
if ref->offset is not byte aligned or ref->size is not known to be
equal to ref->max_size.
(maybe_trim_complex_store): Fix description.
(maybe_trim_constructor_store): Likewise.
(maybe_trim_partially_dead_store): Likewise.


2024-02-27  Eric Botcazou  

* gnat.dg/opt104.ads, gnat.dg/opt104.adb! New test.

-- 
Eric Botcazoudiff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc
index 81b65125409..5869010287c 100644
--- a/gcc/tree-ssa-dse.cc
+++ b/gcc/tree-ssa-dse.cc
@@ -403,11 +403,11 @@ setup_live_bytes_from_ref (ao_ref *ref, sbitmap live_bytes)
   return false;
 }
 
-/* Compute the number of elements that we can trim from the head and
-   tail of ORIG resulting in a bitmap that is a superset of LIVE.
+/* Compute the number of stored bytes that we can trim from the head and
+   tail of REF.  LIVE is the bitmap of stores to REF that are still live.
 
-   Store the number of elements trimmed from the head and tail in
-   TRIM_HEAD and TRIM_TAIL.
+   Store the number of bytes trimmed from the head and tail in TRIM_HEAD
+   and TRIM_TAIL respectively.
 
STMT is the statement being trimmed and is used for debugging dump
output only.  */
@@ -416,10 +416,16 @@ static void
 compute_trims (ao_ref *ref, sbitmap live, int *trim_head, int *trim_tail,
 	   gimple *stmt)
 {
-  /* We use sbitmaps biased such that ref->offset is bit zero and the bitmap
- extends through ref->size.  So we know that in the original bitmap
- bits 0..ref->size were true.  We don't actually need the bitmap, just
- the REF to compute the trims.  */
+  *trim_head = 0;
+  *trim_tail = 0;
+
+  /* We use bitmaps biased such that ref->offset is contained in bit zero and
+ the bitmap extends through ref->max_size and we know that in the original
+ bitmap bits 0 .. ref->max_size were true.  But we need to check that this
+ covers exactly the bytes of REF.  */
+  const unsigned int align = known_alignment (ref->offset);
+  if ((align && align < BITS_PER_UNIT) || !known_eq (ref->size, ref->max_size))
+return;
 
   /* Now identify how much, if any of the tail we can chop off.  */
   HOST_WIDE_INT const_size;
@@ -444,8 +450,6 @@ compute_trims (ao_ref *ref, sbitmap live, int *trim_head, int *trim_tail,
 			   last_orig) <= 0)
 	*trim_tail = 0;
 }
-  else
-*trim_tail = 0;
 
   /* Identify how much, if any of the head we can chop off.  */
   int first_orig = 0;
@@ -503,8 +507,7 @@ compute_trims (ao_ref *ref, sbitmap live, int *trim_head, int *trim_tail,
 	}
 }
 
-  if ((*trim_head || *trim_tail)
-  && dump_file && (dump_flags & TDF_DETAILS))
+  if ((*trim_head || *trim_tail) && dump_file && (dump_flags & TDF_DETAILS))
 {
   fprintf (dump_file, "  Trimming statement (head = %d, tail = %d): ",
 	   *trim_head, *trim_tail);
@@ -513,9 +516,9 @@ compute_trims (ao_ref *ref, sbitmap live, int