diff mbox series

Fix internal error in GIMPLE DSE

Message ID 1958524.PYKUYFuaPT@fomalhaut
State New
Headers show
Series Fix internal error in GIMPLE DSE | expand

Commit Message

Eric Botcazou Feb. 27, 2024, 12:49 p.m. UTC
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 (&const_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  <ebotcazou@adacore.com>

	* 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  <ebotcazou@adacore.com>

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

Comments

Richard Biener Feb. 27, 2024, 1:31 p.m. UTC | #1
On Tue, Feb 27, 2024 at 1:50 PM Eric Botcazou <botcazou@adacore.com> 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 (&const_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  <ebotcazou@adacore.com>
>
>         * 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  <ebotcazou@adacore.com>
>
>         * gnat.dg/opt104.ads, gnat.dg/opt104.adb! New test.
>
> --
> Eric Botcazou
diff mbox series

Patch

diff --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 *trim_head, int *trim_tail,
     }
 }
 
-/* STMT initializes an object from COMPLEX_CST where one or more of the
-   bytes written may be dead stores.  REF is a representation of the
-   memory written.  LIVE is the bitmap of stores that are actually live.
+/* STMT initializes an object from COMPLEX_CST where one or more of the bytes
+   written may be dead stores.  REF is a representation of the memory written.
+   LIVE is the bitmap of stores to REF that are still live.
 
    Attempt to rewrite STMT so that only the real or imaginary part of
    the object is actually stored.  */
@@ -554,11 +557,10 @@  maybe_trim_complex_store (ao_ref *ref, sbitmap live, gimple *stmt)
 }
 
 /* STMT initializes an object using a CONSTRUCTOR where one or more of the
-   bytes written are dead stores.  ORIG is the bitmap of bytes stored by
-   STMT.  LIVE is the bitmap of stores that are actually live.
+   bytes written are dead stores.  REF is a representation of the memory
+   written.  LIVE is the bitmap of stores to REF that are still live.
 
-   Attempt to rewrite STMT so that only the real or imaginary part of
-   the object is actually stored.
+   Attempt to rewrite STMT so that only the live stores are performed.
 
    The most common case for getting here is a CONSTRUCTOR with no elements
    being used to zero initialize an object.  We do not try to handle other
@@ -780,9 +782,9 @@  maybe_trim_memstar_call (ao_ref *ref, sbitmap live, gimple *stmt)
     }
 }
 
-/* STMT is a memory write where one or more bytes written are dead
-   stores.  ORIG is the bitmap of bytes stored by STMT.  LIVE is the
-   bitmap of stores that are actually live.
+/* STMT is a memory write where one or more bytes written are dead stores.
+   REF is a representation of the memory written.  LIVE is the bitmap of
+   stores to REF that are still live.
 
    Attempt to rewrite STMT so that it writes fewer memory locations.  Right
    now we only support trimming at the start or end of the memory region.