Re: regrename.c RTL sharing bug

[Andrew Jenner <andrew at codesourcery dot com>]

Hi Mark,

Mark Mitchell wrote:
> I don't quite understand what's going on here.  The copy_rtx function 
> won't copy cc0, IIUC, so I don't understand the whole situation.  Would 
> you be able to walk through the whole process of what goes wrong, with 
> gruesome before-and-after pictures of the mangled instructions?

Before it gets mangled, the insn looks like this:

(insn:HI 86 84 73 2 ra.f90:6 (parallel [
    (set (reg:CC_NOOV 24 cc)
        (compare:CC_NOOV (and:SI (lshiftrt:SI (reg:SI 2 r2 [203])
                    (const_int 31 [0x1f]))
                (reg:SI 3 r3 [204]))
            (const_int 0 [0x0])))
    (set (reg:SI 0 r0 [206])
        (and:SI (lshiftrt:SI (reg:SI 2 r2 [203])
                (const_int 31 [0x1f]))
            (reg:SI 3 r3 [204])))
]) 265 {*arith_shiftsi_compare0} (nil))

At this point, *recog_data.dup_loc[0] is:

(and:SI (lshiftrt:SI (reg:SI 2 r2 [203])
        (const_int 31 [0x1f]))
    (reg:SI 3 r3 [204]))

(which appears twice in the insn as you can see.)

In step 2 of  build_def_use(), the dup_loc[] entries are copied to
old_dups[] and then both the dup_loc[]s and operands are munged into (cc0):

>          /* Step 2: Close chains for which we have reads outside operands.
>             We do this by munging all operands into CC0, and closing
>             everything remaining.  */
>
>          for (i = 0; i < n_ops; i++)
>            {
>              old_operands[i] = recog_data.operand[i];
>              /* Don't squash match_operator or match_parallel here, since
>                 we don't know that all of the contained registers are
>                 reachable by proper operands.  */
>              if (recog_data.constraints[i][0] == '\0')
>                continue;
>              *recog_data.operand_loc[i] = cc0_rtx;
>            }
>           for (i = 0; i < recog_data.n_dups; i++)
>             {
>               old_dups[i] = *recog_data.dup_loc[i];
>               *recog_data.dup_loc[i] = cc0_rtx;
>             }

The insn now looks like this:

(insn:HI 86 84 73 2 ra.f90:6 (parallel [
            (set (reg:CC_NOOV 24 cc)
                (compare:CC_NOOV (and:SI (lshiftrt:SI (cc0)
                            (cc0))
                        (cc0))
                    (const_int 0 [0x0])))
            (set (cc0)
                (cc0))
        ]) 265 {*arith_shiftsi_compare0} (expr_list:REG_DEAD (reg:SI 3
r3 [204])
        (expr_list:REG_DEAD (reg:SI 2 r2 [203])
            (nil))))

Then the dup_loc[] entries are restored, copying them in the process.

>         for (i = 0; i < recog_data.n_dups; i++)
>           *recog_data.dup_loc[i] = copy_rtx (old_dups[i]);

However, the operands are still munged into cc0, so when they are
copied, the copies have cc0 operands.

Then the operands are put back:

>          for (i = 0; i < n_ops; i++)
>            *recog_data.operand_loc[i] = old_operands[i];

But only the originals, not the copies. So now the insn is broken:

(insn:HI 86 84 73 2 ra.f90:6 (parallel [
            (set (reg:CC_NOOV 24 cc)
                (compare:CC_NOOV (and:SI (lshiftrt:SI (reg:SI 2 r2 [203])
                            (const_int 31 [0x1f]))
                        (reg:SI 3 r3 [204]))
                    (const_int 0 [0x0])))
            (set (reg:SI 0 r0 [206])
                (and:SI (cc0)
                    (cc0)))
        ]) 265 {*arith_shiftsi_compare0} (expr_list:REG_DEAD (reg:SI 3
r3 [204])
        (expr_list:REG_DEAD (reg:SI 2 r2 [203])
            (nil))))

Having written this out I now understand it a bit better than when I
originally debugged it, and a number of other possible fixes suggest 
themselves (like restoring the operands before restoring the dup_locs). 
I'm not sure if this would be preferred over the fix that Jan and I 
originally agreed upon (copying the RTX before changing anything) which 
is known to work.

I also just noticed that further down the function (in Step 5) there is 
an almost identical piece of doesn't copy the old_dups. This suggests 
that my first fix (just removing the call to copy_rtx() altogether) 
might be the better one. I also tested that one quite thoroughly so I'm 
pretty sure it doesn't break anything (but I hesitated to remove it 
altogether because I didn't know why it was there in the first place).

Andrew
--
Andrew Jenner
CodeSourcery
andrew@codesourcery.com
(650) 331-3385 x728