This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 12/14/06, Eric Botcazou <ebotcazou@libertysurf.fr> wrote:
> What do you think of the attached patch?

I think that the gcc_assert is too strong:

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


set_unique_reg_note can
legitimately punt and, in this case, everything will be fine, so no need to
stop the compiler.

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.

Therefore it seems to me if s_u_r_n punts if called from cse_insn, it
would indicate a bug in cse_insn.  Hence I put the assert there.


You should also make the guard more consistent with

              /* With non-call exceptions, if this was an insn that could
                 trap, we may have made it non-throwing now.  For example
                 we may have replaced a load with a register.  */
              if (flag_non_call_exceptions
                  && insn == BB_END (BLOCK_FOR_INSN (insn)))
                purge_dead_edges (BLOCK_FOR_INSN (insn));

a few lines above: either both invoke may_trap_p (1 or 2 times), or both don't
invoke it.

I agree that would be better.


Unfortunately, I _can't_ call it in the case you quote, except before
doing the validate_replacement_change on the line:

else if (validate_change (insn, &SET_SRC (sets[i].rtl), trial, 0))

After that validate_change, may_trap_p (PATTERN(INSN)) would return
false even if the insn could trap before the validate_change.

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.


However it seems to me that there are more cases in cse_insn that could cause
the compiler to stop on

  /* If there are dead edges to purge, we haven't properly updated
     the CFG incrementally.  */
  gcc_assert (!purge_all_dead_edges ());

Hence the question: can't we devise a general mechanism to update the CFG
incrementally in all cases, instead of handling them one by one?

Yes, I considered that too. I don't know if there are more cases where cse_insn changes insns in ways that cause CFG changes. I certainly hope not.

In case we find more examples of this, we could put the whole CFG
updating into the look over all BB insns in cse_extended_basic_block.

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.

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.

Gr.
Steven


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]