[RFA] [PATCH][PR tree-optimization/59749] Fix recently introduced ree bug

Jeff Law law@redhat.com
Wed Jan 15 17:56:00 GMT 2014


On 01/13/14 00:34, Jakub Jelinek wrote:
> On Sun, Jan 12, 2014 at 11:21:59PM -0700, Jeff Law wrote:
>> --- a/gcc/ree.c
>> +++ b/gcc/ree.c
>> @@ -297,6 +297,13 @@ combine_set_extension (ext_cand *cand, rtx curr_insn, rtx *orig_set)
>>     else
>>       new_reg = gen_rtx_REG (cand->mode, REGNO (SET_DEST (*orig_set)));
>>
>> +  /* We're going to be widening the result of DEF_INSN, ensure that doing so
>> +     doesn't change the number of hard registers needed for the result.  */
>> +  if (HARD_REGNO_NREGS (REGNO (new_reg), cand->mode)
>> +      != HARD_REGNO_NREGS (REGNO (SET_SRC (*orig_set)),
>
> Note you can use orig_src instead of SET_SRC (*orig_set) here.
Yea.  A little manual CSE never hurts.  Fixed.


>
>> +			   GET_MODE (SET_DEST (*orig_set))))
>> +	return false;
>> +
>>     /* Merge constants by directly moving the constant into the register under
>>        some conditions.  Recall that RTL constants are sign-extended.  */
>>     if (GET_CODE (orig_src) == CONST_INT
>
> Are you sure the above is needed even for the
> REGNO (new_reg) == REGNO (SET_DEST (*orig_set))
> && REGNO (new_reg) == REGNO (orig_src) case?
It wasn't clear when I was reviewing the code, so I took the 
conservative approach of always rejecting when the # hard regs changes 
as a result of widening.

>
> (set (reg:SI 3) (something:SI))
> (set (reg:SI 2) (expression:SI))	// def_insn
> (use (reg:SI 3))
> (set (reg:DI 3) (sign_extend:DI (reg:SI 2)))
>
> So, perhaps if we wanted to handle the HARD_REGNO_NREGS != HARD_REGNO_NREGS
> case when all 3 REGNOs are the same, we'd need to limit it to the case where
> cand->insn and curr_insn are in the same bb, DF_INSN_LUID of curr_insn
> is smaller than DF_INSN_LUID of cand->insn and the extra hard regs aren't
> used between the two.  Perhaps not worth it?
I doubt it's worth it.  A size change here is pretty unusual.

>
> BTW, I'm surprised to hear that it triggers in the testsuite already (for
> the 3 REGNOs the same case, or different?), is that on x86_64 or i?86?
> Do you have an example?  I'm surprised that we'd have post reload a pattern
> that extends into multiple hard registers.
There were only a couple on x86_64, both related to handling vector 
regs.  They would actually fail later, but they tripped the changing 
size check.

Here's one example:

(insn 16 15 17 2 (set (reg:V4SI 21 xmm0 [99])
         (vec_select:V4SI (reg:V8SI 22 xmm1 [orig:85 vect__3.443 ] [85])
             (parallel [
                     (const_int 4 [0x4])
                     (const_int 5 [0x5])
                     (const_int 6 [0x6])
                     (const_int 7 [0x7])
                 ]))) j.c:26 1926 {vec_extract_hi_v8si}
      (nil))
(insn 17 16 18 2 (set (reg:V4DI 21 xmm0 [orig:98 vect__4.444 ] [98])
         (sign_extend:V4DI (reg:V4SI 21 xmm0 [99]))) j.c:26 2583 
{avx2_sign_extendv4siv4di2}
      (nil))



Changing (reg:V4SI xmm0) to (reg:V4DI xmm0) changes the number of hard 
regs and thus tripped the check.

The transformation would actually fail later when it tried to actually 
combine the extension and vec_select into this insn:

(insn 16 15 17 2 (set (reg:V4DI 21 xmm0)
         (sign_extend:V4DI (vec_select:V4SI (reg:V8SI 22 xmm1 [orig:85 
vect__3.443 ] [85])
                 (parallel [
                         (const_int 4 [0x4])
                         (const_int 5 [0x5])
                         (const_int 6 [0x6])
                         (const_int 7 [0x7])
                     ])))) j.c:26 -1

I guess I could run a -m32 multilib test and see if I can get it to 
trigger in on something that will create a valid insn.


Jeff



More information about the Gcc-patches mailing list