On 13-01-19 11:57 AM, Steven Bosscher wrote:
On Sat, Jan 19, 2013 at 1:15 AM, Vladimir Makarov wrote:
On 13-01-17 6:45 PM, Steven Bosscher wrote:
Hello Vlad,
Attached is my attempt to fix PR55934, an error recovery issue in LRA
with incorrect constraints in an asm.
I'm not 100% sure this is all correct (especially the LRA insn data
invalidating in lra-assigns.c) but it appears to fix the PR without
introducing test suite failures.
The code is correct but call lra_invalidate_insn_data is not necessary as
the same thing will be done in lra_update_insn_recog_data (if what
lra_invalidate_insn_data does is not done yet) .
That is what I expected, too. My first attempts didn't have the
lra_invalidate_insn_data call.
But I think lra_update_insn_recog_data calls lra_invalidate_insn_data
for asms. lra_invalidate_insn_data is called if there is existing
recog data but the insn code has changed:
if ((data = lra_insn_recog_data[uid]) != NULL
&& data->icode != INSN_CODE (insn))
{
invalidate_insn_data_regno_info (data, insn, get_insn_freq (insn));
invalidate_insn_recog_data (uid);
data = NULL;
}
For an asm, INSN_CODE==-1, so "data->icode != INSN_CODE (insn)" is
always false, and lra_invalidate_insn_data is never called. The result
is an ICE:
pr55512-3.c:15:1: internal compiler error: in
lra_update_insn_recog_data, at lra.c:1263
lra.c:1263 lra_assert (nop == data->insn_static_data->n_operands);
So adding the additional
call is harmless as the result will be the same.
Given my explanation above, do you think we should make
lra_update_insn_recog_data handle asms as a special case? E.g.:
Index: lra.c
===================================================================
--- lra.c (revision 195104)
+++ lra.c (working copy)
@@ -1239,7 +1239,8 @@ lra_update_insn_recog_data (rtx insn)
check_and_expand_insn_recog_data (uid);
if ((data = lra_insn_recog_data[uid]) != NULL
- && data->icode != INSN_CODE (insn))
+ && (data->icode != INSN_CODE (insn)
+ || asm_noperands (PATTERN (insn)) >= 0))
{
invalidate_insn_data_regno_info (data, insn, get_insn_freq (insn));
invalidate_insn_recog_data (uid);
Or just keep the lra_invalidate_insn_data call?
Yes, I guess you are right -- I missed a special treatment of asm in
lra_update_insn_recog_data. I'd prefer the above change than just
keeping lra_invalidate_insn_data call. I think it is more safe solution
for other parts of LRA code.
Thanks, Steven.
