On Thu, Nov 23, 2023 at 10:43 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> Hi!
>
> As the following testcase shows, there are some bugs in the
> -fnon-call-exceptions bit-field load lowering. In particular, there
> is a case where we want to emit a load early in the initialization
> (before m_init_gsi) and because that load might throw exception, need
> to split block after the load so that it has an EH edge.
> Now, across this splitting, we have m_init_gsi, save_gsi (something
> we put back into m_gsi afterwards) statement iterators and m_preheader_bb
> which is used to determine the pre-header edge of a loop (if any).
> As the testcase shows, both of these statement iterators and m_preheader_bb
> as well need adjustments if the block was split. If the stmt iterators
> refer to a statement, they need to be updated so that if the statement is
> in the bb after the split gsi_bb and gsi_seq is updated, otherwise they
> ought to be the start of the new (second) bb.
> Similarly, m_preheader_bb should be updated to the second bb if it was
> the first before. Other spots where we insert something before m_init_gsi
> don't split blocks in there and are fine.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Wheee ...
> 2023-11-23 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/112668
> * gimple-lower-bitint.cc (bitint_large_huge::handle_load): When
> splitting gsi_bb (m_init_gsi) basic block, update m_preheader_bb
> if needed, fix up update of m_init_gsi and update saved m_gsi
> as well if needed.
>
> * gcc.dg/bitint-40.c: New test.
>
> --- gcc/gimple-lower-bitint.cc.jj 2023-11-14 10:52:16.000000000 +0100
> +++ gcc/gimple-lower-bitint.cc 2023-11-22 14:34:17.327140002 +0100
> @@ -1687,7 +1687,22 @@ bitint_large_huge::handle_load (gimple *
> edge e = split_block (gsi_bb (m_gsi), g);
> make_edge (e->src, eh_edge->dest, EDGE_EH)->probability
> = profile_probability::very_unlikely ();
> - m_init_gsi.bb = e->dest;
> + m_init_gsi = gsi_last_bb (e->dest);
shouldn't that be gsi_start_bb (e->dest) if we want to continue inserting
before the "old" stmt?
> + if (!gsi_end_p (m_init_gsi))
> + gsi_next (&m_init_gsi);
That would always put it at the end?
> + if (gsi_bb (save_gsi) == e->src)
> + {
> + if (gsi_end_p (save_gsi))
> + {
> + save_gsi = gsi_last_bb (e->dest);
> + if (!gsi_end_p (save_gsi))
> + gsi_next (&save_gsi);
> + }
> + else
> + save_gsi = gsi_for_stmt (gsi_stmt (save_gsi));
uhm. It might be better to instead of doing save_gsi = m_gsi
save gsi_stmt () and gsi_bb () to avoid accessing the now
possibly invalid iterator?
If there were only one iterator I'd say we want a
split_block_{after,before} (&gsi);
which hides the detail of updating the iterator. But you have the
additional issue of possibly updating another iterator where as said
the better solution would be to reconstruct it from a gimple *
(or basic_block if at end). Maybe we can have a
gsi_update_after_spli_block (&gsi, basic_block-that-was-split)?
If you think any of this would be an improvement (but also see
the gsi_last_bb vs gsi_start issue) feel free to improve.
Otherwise OK as-is.
Richard.
> + }
> + if (m_preheader_bb == e->src)
> + m_preheader_bb = e->dest;
> }
> }
> m_gsi = save_gsi;
> --- gcc/testsuite/gcc.dg/bitint-40.c.jj 2023-11-22 13:47:12.380580107 +0100
> +++ gcc/testsuite/gcc.dg/bitint-40.c 2023-11-22 14:35:50.225842768 +0100
> @@ -0,0 +1,29 @@
> +/* PR middle-end/112668 */
> +/* { dg-do compile { target bitint } } */
> +/* { dg-options "-std=c23 -fnon-call-exceptions" } */
> +
> +#if __BITINT_MAXWIDTH__ >= 156
> +struct T156 { _BitInt(156) a : 2; unsigned _BitInt(156) b : 135;
> _BitInt(156) c : 2; };
> +extern void foo156 (struct T156 *);
> +
> +unsigned _BitInt(156)
> +bar156 (int i)
> +{
> + struct T156 r156[12];
> + foo156 (&r156[0]);
> + return r156[i].b;
> +}
> +#endif
> +
> +#if __BITINT_MAXWIDTH__ >= 495
> +struct T495 { _BitInt(495) a : 2; unsigned _BitInt(495) b : 471;
> _BitInt(495) c : 2; };
> +extern void foo495 (struct T495 *r495);
> +
> +unsigned _BitInt(495)
> +bar495 (int i)
> +{
> + struct T495 r495[12];
> + foo495 (r495);
> + return r495[i].b;
> +}
> +#endif
>
> Jakub
>
