[PATCH,RFC] CSE path following on basic blocks

[Eric Botcazou]

> I'm not sure if we want to do this, though.  We could follow the
> fallthrough path even if flag_cse_follow_jumps is not set (which is
> indeed what CSE currently does). Or we could make CSE purely a local
> (i.e. intra basic block) pass if flag_cse_follow_jumps is not set.
> Personally, I don't think it matters much, but making it purely local
> is slightly easier and a lot more consistent.

I agree that, in the cfglayout implementation, following one edge and not the 
other is kind of strange, so it would indeed make sense to turn f_c_f_j into 
the predicate for pure locality.  But the change should be clearly stated in 
the code and then cse_find_path not invoked if f_c_f_j if false.

> > *************** cse_insn (rtx insn, rtx libcall_insn)
> > *** 5599,5606 ****
> >             This section previously turned the REG_EQUIV into a REG_EQUAL
> >             note.  We cannot do that because REG_EQUIV may provide an
> >             uninitialized stack slot when REG_PARM_STACK_SPACE is used. 
> > */ !
> > !         if (prev != 0 && NONJUMP_INSN_P (prev)
> >              && GET_CODE (PATTERN (prev)) == SET
> >              && SET_DEST (PATTERN (prev)) == SET_SRC (sets[0].rtl)
> >              && ! find_reg_note (prev, REG_EQUIV, NULL_RTX))
> > --- 5609,5616 ----
> >             This section previously turned the REG_EQUIV into a REG_EQUAL
> >             note.  We cannot do that because REG_EQUIV may provide an
> >             uninitialized stack slot when REG_PARM_STACK_SPACE is used. 
> > */ !         gcc_assert (prev);
> > !         if (NONJUMP_INSN_P (prev)
> >              && GET_CODE (PATTERN (prev)) == SET
> >              && SET_DEST (PATTERN (prev)) == SET_SRC (sets[0].rtl)
> >              && ! find_reg_note (prev, REG_EQUIV, NULL_RTX))
> >
> > Please explain.
>
> Well, we know that prev == bb_head, and a basic block with a NULL
> bb_head can't occur.  The rest is just what the code was before.

Then the gcc_assert seems pretty useless there too.

> The old code does think it computes an upper bound, but if you remove
> the 500 from CSE as it is on the trunk, it will also ICE, usually on
> small functions (e.g. I had a test case with max_qty == 3, and we
> allocated 6 qtys).  I don't know where the extra qty's come from yet.

OK, thanks for the clarification.  The compiler doesn't really write past the 
end of qty_table, right?  I was somewhat confused by this assertion in your 
comment, because sanity checks are supposed to prevent that.

> No, I will not do this for the current patch (but I will look into it
> later because I need to fix this anyway if I'm going to save/restore
> the hash table at the end of each basic block).

Then why multiply by MAX_RECOG_OPERANDS?  Why not just keep the almost 15-year 
old trick?

> Thanks for looking at the patch!

I generally avoid bugging people for nothing. :-)

-- 
Eric Botcazou