Re: [PATCH] Make CSE path following use the CFG

[Eric Botcazou <ebotcazou at libertysurf dot fr>]

> You really are an "I can always do better" kinda guy, no? :-)

I presume so. :-)  More seriously, substantial rewrites of cse.c are pretty 
rare so I think that we can be extra careful when one of them is being made.

> If you look at set_unique_reg_note, you see that there are no cases
> where cse_insn can call it and expect a NULL_RTX result.
>
> There is the n_sets == 1 check in cse_insn, so the multiple sets check
> in s_u_r_n can never trigger; and the datum argument to s_u_r_n can
> never be a ASM_OPERANDS because cse_insn only calls it to put a
> REG_EQUAL note with a constant.

I think that there is a (theoritical) hole for patterns like

  [(set (reg) (src)
   (set (pc) (label)]

because n_sets == 1 for CSE in this case but multiple_sets will return true.

> Calling may_trap_p on the insn at the start of cse_insn, and using
> that later in the apply_change_group case (i.e. the code you quoted),
> that would work.  Not calling may_trap_p in the new REG_EQUAL case
> would also work.  I have no preference either way.

I don't have either, I'd just use the same idiom in both cases.

> In fact, I considered that earlier to fix the Java problem I had after
> the TER changes, and again to fix you Ada bug report. I opted for
> updating the CFG in cse_insn because it seems to me that this is the
> most transparent: Make all changes necessary after an insn change as
> close as possible to the code where the insn change occurs.

Then why not updating it at the very end of cse_insn once for all?

> Should more cases surface of this assert being triggerd, I guess we'll
> just have to update the CFG in cse_extended_basic_block after all.

How many more cases? :-)

-- 
Eric Botcazou