PR inline-asm/55934

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.:



Or just keep the lra_invalidate_insn_data call?

Ciao!
Steven

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.

On Mon, Jan 21, 2013 at 5:22 PM, Vladimir Makarov wrote:
> 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.

I agree, but unfortunately the compiler does not...

With that lra.c change, I get extra fails:

+FAIL: gcc.target/i386/20011029-2.c (internal compiler error)
+FAIL: gcc.target/i386/20011029-2.c (test for excess errors)
+FAIL: gcc.target/i386/pr21291.c (internal compiler error)
+FAIL: gcc.target/i386/pr21291.c (test for excess errors)

These are constrain_operands(1) failures in check_rtl. Apparently some
relevant info is lost in lra_update_insn_recog_data. Not sure how to
debug this...

Ciao!
Steven