This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Make CSE path following use the CFG
On 12/14/06, Eric Botcazou <firstname.lastname@example.org> 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? :-)
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. */
&& 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
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.