regrename.c RTL sharing bug
Andrew Jenner
andrew@codesourcery.com
Sat Jun 14 04:34:00 GMT 2008
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
More information about the Gcc-patches
mailing list