Fix internal error in GIMPLE DSE
Checks
Commit Message
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
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
@@ -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.