On Thu, Nov 23, 2023 at 11:01:08AM +0100, Richard Biener wrote:
> > --- 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.
The code has 2 iterators and pretty much everything in the pass inserts
statements before iterator.
m_gsi is the iterator before which everything is inserted, initially
initialized to
m_gsi = gsi_for_stmt (stmt);
where stmt is the statement we are lowering, but updated in many places
when splitting a bb etc. So, it needs to behave right for the insert
before behavior, gsi_end_p means insert at the end of bb.
Then m_init_gsi is initially one statement earlier, so insert code
after that statement instead:
m_init_gsi = m_gsi;
gsi_prev (&m_init_gsi);
with gsi_end_p meaning insert at the start of bb.
Because all the pass infrastructure is for inserting before rather than
after, when inserting (temporarily) after m_init_gsi, it does
gimple_stmt_iterator save_gsi = m_gsi;
m_gsi = m_init_gsi;
if (gsi_end_p (m_gsi))
m_gsi = gsi_after_labels (gsi_bb (m_gsi));
else
gsi_next (&m_gsi);
and then it can insert before m_gsi and finally when done there
m_gsi = save_gsi;
The problematic splitting of the bb is during this temporary override
to insert stuff after m_init_gsi.
For the save_gsi update, I believe I'm reconstructing it from
the stmt if any and set to gsi_end_p if it was gsi_end_p before:
+ 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));
I don't know how else to re-create a gsi_end_p iterator from a bb
(other option I know of would be gsi_start_bb and gsi_prev if !gsi_end_p,
but stmt after gsi_last_bb seems to match more the intent).
Now, regarding m_init_gsi, I think I'll need to play around, maybe
I should have in the end insert after and update behavior rather than
insert after, and that could be achieved by adding
m_init_gsi = m_gsi;
gsi_prev (&m_init_gsi);
before the
m_gsi = save_gsi;
restore in all the 3 places and then no other updates of m_init_gsi would be
needed. Except gsi_prev likely won't like the gsi_end_p (m_gsi) case,
so maybe
m_init_gsi = m_gsi;
if (gsi_end_p (m_init_gsi))
m_init_gsi = gsi_last_bb (gsi_bb (m_init_gsi));
else
gsi_prev (&m_init_gsi);
m_gsi = save_gsi;
Jakub
