[PATCH, rtl]: Remove invalid assert from caller-save.c

[PATCH, rtl]: Remove invalid assert from caller-save.c

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 889 bytes --]

Hello!

In function reg_save_code, we detect availability of certain move insns 
with:

   ...
   cached_reg_save_code[reg][mode] = recog_memoized (saveinsn);
   ...

and further down the code, we assert that:

   ...
   gcc_assert (cached_reg_save_code[reg][mode]);
   ...

However, value of 0 is perfectly valid return value for recog_memoized. 
I learned this the hard way, as shown from generated insn-recog.c:

static int
recog_1 (rtx x0 ATTRIBUTE_UNUSED,
     rtx insn ATTRIBUTE_UNUSED,
     int *pnum_clobbers ATTRIBUTE_UNUSED)
{
   ...
     {
       return 0;  /* *movqi */
     }
   x1 = XEXP (x0, 0);
   goto L237;
   ...
  ret0:
   return -1;
}

Attached one-liner fixes this oversight.

2009-11-24  Uros Bizjak <ubizjak@gmail.com>

     * caller-save.c (reg_save_code): Remove invalid gcc_assert.

Patch was tested on x86_64-pc-linux-gnu.

OK for 4.3, 4.4 and mainline?

Uros.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 416 bytes --]

Index: caller-save.c
===================================================================
--- caller-save.c	(revision 154503)
+++ caller-save.c	(working copy)
@@ -175,7 +175,6 @@ reg_save_code (int reg, enum machine_mod
       cached_reg_save_code[reg][mode] = -1;
       cached_reg_restore_code[reg][mode] = -1;
     }
-  gcc_assert (cached_reg_save_code[reg][mode]);
   return cached_reg_save_code[reg][mode];
 }
 

Re: [PATCH, rtl]: Remove invalid assert from caller-save.c

[Eric Botcazou]

> However, value of 0 is perfectly valid return value for recog_memoized.
> I learned this the hard way, as shown from generated insn-recog.c:
>
> static int
> recog_1 (rtx x0 ATTRIBUTE_UNUSED,
>      rtx insn ATTRIBUTE_UNUSED,
>      int *pnum_clobbers ATTRIBUTE_UNUSED)
> {
>    ...
>      {
>        return 0;  /* *movqi */
>      }
>    x1 = XEXP (x0, 0);
>    goto L237;
>    ...
>   ret0:
>    return -1;
> }
>
> Attached one-liner fixes this oversight.

Well, all the logic would need to be rewritten then:

static int
reg_save_code (int reg, enum machine_mode mode)
{
  bool ok;
  if (cached_reg_save_code[reg][mode])
     return cached_reg_save_code[reg][mode];

static int
reg_restore_code (int reg, enum machine_mode mode)
{
  if (cached_reg_restore_code[reg][mode])
     return cached_reg_restore_code[reg][mode];

Can't you just avoid having "movqi" associated with 0?

-- 
Eric Botcazou

Re: [PATCH, rtl]: Remove invalid assert from caller-save.c

[Uros Bizjak]

On 11/24/2009 10:42 PM, Eric Botcazou wrote:
>> However, value of 0 is perfectly valid return value for recog_memoized.
>> I learned this the hard way, as shown from generated insn-recog.c:
>>
>> static int
>> recog_1 (rtx x0 ATTRIBUTE_UNUSED,
>>       rtx insn ATTRIBUTE_UNUSED,
>>       int *pnum_clobbers ATTRIBUTE_UNUSED)
>> {
>>     ...
>>       {
>>         return 0;  /* *movqi */
>>       }
>>     x1 = XEXP (x0, 0);
>>     goto L237;
>>     ...
>>    ret0:
>>     return -1;
>> }
>>
>> Attached one-liner fixes this oversight.
>>      
> Well, all the logic would need to be rewritten then:
>
> static int
> reg_save_code (int reg, enum machine_mode mode)
> {
>    bool ok;
>    if (cached_reg_save_code[reg][mode])
>       return cached_reg_save_code[reg][mode];
>
> static int
> reg_restore_code (int reg, enum machine_mode mode)
> {
>    if (cached_reg_restore_code[reg][mode])
>       return cached_reg_restore_code[reg][mode];
>
> Can't you just avoid having "movqi" associated with 0?
>    

No problem for me now, but some other poor soul will surely hit this bug 
again. ;)

Uros.

Re: [PATCH, rtl]: Remove invalid assert from caller-save.c

[Richard Henderson]

On 11/24/2009 01:42 PM, Eric Botcazou wrote:
> Well, all the logic would need to be rewritten then:
>
> static int
> reg_save_code (int reg, enum machine_mode mode)
> {
>    bool ok;
>    if (cached_reg_save_code[reg][mode])
>       return cached_reg_save_code[reg][mode];
>
> static int
> reg_restore_code (int reg, enum machine_mode mode)
> {
>    if (cached_reg_restore_code[reg][mode])
>       return cached_reg_restore_code[reg][mode];
>
> Can't you just avoid having "movqi" associated with 0?

Can't we simply arrange for cached_reg_restore_code to
be initialized to -1?


r~

Re: [PATCH, rtl]: Remove invalid assert from caller-save.c

[Eric Botcazou]

> Can't we simply arrange for cached_reg_restore_code to
> be initialized to -1?

Not really, -1 already means something else.  No doubt this can be fixed, but 
it's no one-liner so can certainly wait for 4.6.

-- 
Eric Botcazou

Re: [PATCH, rtl]: Remove invalid assert from caller-save.c

[Richard Henderson]

On 11/28/2009 12:45 PM, Eric Botcazou wrote:
>> Can't we simply arrange for cached_reg_restore_code to
>> be initialized to -1?
>
> Not really, -1 already means something else.  No doubt this can be fixed, but
> it's no one-liner so can certainly wait for 4.6.

Ah.  In which case for 4.5 it might be best to just
rearrange a pattern in i386.md to the top.  Perhaps
something like "blockage" or "nop" would be best.


r~

Re: [PATCH, rtl]: Remove invalid assert from caller-save.c

[Uros Bizjak]

On 11/28/2009 09:51 PM, Richard Henderson wrote:

>>> be initialized to -1?
>>
>> Not really, -1 already means something else.  No doubt this can be 
>> fixed, but
>> it's no one-liner so can certainly wait for 4.6.
> Can't we simply arrange for cached_reg_restore_code to
>
> Ah.  In which case for 4.5 it might be best to just
> rearrange a pattern in i386.md to the top.  Perhaps
> something like "blockage" or "nop" would be best.

The problem is not directly related to x86, it was triggered by testing 
some other stuff. I just thought that the problem is not yet widely 
known, and it might be of interest to other people.

I have "fixed" this by putting "nop" as the first insn. IMO, it is also 
somehow intuitive to have "nop" at place 0.

Uros.