[PATCH][RFA][LRA] Don't try to break down subreg expressions if insn already matches

[PATCH][RFA][LRA] Don't try to break down subreg expressions if insn already matches

[Kyrill Tkachov]

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

Hi all,

In my tree added a pattern to the arm backend that's supposed to match:
(set (reg:SI r0)
      (subreg:SI
        (plus:DI
          (mult:DI (sign_extend:DI (reg:SI r1))
                   (sign_extend:DI (reg:SI r2)))
        (const_int 2147483648 [0x80000000])) 4))

That is, take two SImode regs, sign-extend to DImode, multiply in DImode,
add a const_int and take the most significant SImode subreg.
Combine matches it fine but I get an ICE in lra:
0xa665a5 crash_signal
         $SRC/gcc/toplev.c:383
0x91ec1c lra_emit_add(rtx_def*, rtx_def*, rtx_def*)
         $SRC/gcc/lra.c:418
0x91ef8a lra_emit_move(rtx_def*, rtx_def*)
         $SRC/gcc/lra.c:528
0x9279b2 insert_move_for_subreg
         $SRC/gcc/lra-constraints.c:1371
0x931bdb simplify_operand_subreg
         $SRC/gcc/lra-constraints.c:1507
0x931bdb curr_insn_transform
         $SRC/gcc/lra-constraints.c:3437
0x934793 lra_constraints(bool)
         $SRC/gcc/lra-constraints.c:4451
0x9217d0 lra(_IO_FILE*)
         $SRC/gcc/lra.c:2292
0x8e0dba do_reload
         $SRC/gcc/ira.c:5418
0x8e0dba execute
         $SRC/gcc/ira.c:5589


I *think* the problem is that simplify_operand_subreg tries to break down
this complex RTX involving the subreg into separate MULT/PLUS operations
and breaking down the line.

If I apply the attached patch that stops trying to simplify the subreg
expression if the whole thing matches something already my code compiles
succesfully, and passes arm testing and x86_64 bootstrap.
However, I'm concerned that calling recog_memoized is too heavy-handed,
is there a better approach?
Or is this even the right way to go about this?

I'm also attaching the patch to the arm backend that can be used to
reproduce this ICE with the following C-code at -O2:

int
smmulr (int a, int b)
{
   return ((long long)a * b + 0x80000000) >> 32;
}


Thanks,
Kyrill

2015-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

	* lra-constraints.c (simplify_operand_subreg): Do not try to simplify
	subreg if the pattern already matches.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: lra-subreg.patch --]
[-- Type: text/x-patch; name=lra-subreg.patch, Size: 1279 bytes --]

commit e21706a7e06aace9fb76040867561f6fac748ab8
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Tue Feb 10 10:20:07 2015 +0000

    [LRA][tmp] Do no try to simplify into subregs if whole insn has a match

diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index 827c453..5cb0422 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -1467,7 +1467,8 @@ simplify_operand_subreg (int nop, machine_mode reg_mode)
   /* Force a reload of the SUBREG_REG if this is a constant or PLUS or
      if there may be a problem accessing OPERAND in the outer
      mode.  */
-  if ((REG_P (reg)
+  if (recog_memoized (curr_insn) < 0
+       && ((REG_P (reg)
        && REGNO (reg) >= FIRST_PSEUDO_REGISTER
        && (hard_regno = lra_get_regno_hard_regno (REGNO (reg))) >= 0
        /* Don't reload paradoxical subregs because we could be looping
@@ -1479,7 +1480,7 @@ simplify_operand_subreg (int nop, machine_mode reg_mode)
        /* Don't reload subreg for matching reload.  It is actually
 	  valid subreg in LRA.  */
        && ! LRA_SUBREG_P (operand))
-      || CONSTANT_P (reg) || GET_CODE (reg) == PLUS || MEM_P (reg))
+      || CONSTANT_P (reg) || GET_CODE (reg) == PLUS || MEM_P (reg)))
     {
       enum reg_class rclass;
 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: arm-smmulr.patch --]
[-- Type: text/x-patch; name=arm-smmulr.patch, Size: 1771 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index c13e9b2..8cf8776 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -1549,6 +1549,22 @@ (define_insn "*mulsidi3_v6"
    (set_attr "predicable_short_it" "no")]
 )
 
+(define_insn "*mulsidi3si_round_v6"
+  [(set (match_operand:SI 0 "s_register_operand" "=r")
+          (match_operator:SI 3 "subreg_highpart_operator"
+            [(plus:DI
+               (mult:DI
+                 (sign_extend:DI (match_operand:SI 1 "s_register_operand" "%r"))
+                 (sign_extend:DI (match_operand:SI 2 "s_register_operand" "r")))
+               (const_int 2147483648))
+]))]
+  "TARGET_32BIT && arm_arch6"
+  "smmulr%?\\t%0, %1, %2"
+  [(set_attr "type" "smmulr")
+   (set_attr "predicable" "yes")
+   (set_attr "predicable_short_it" "no")]
+)
+
 (define_expand "umulsidi3"
   [(set (match_operand:DI 0 "s_register_operand" "")
 	(mult:DI
diff --git a/gcc/config/arm/predicates.md b/gcc/config/arm/predicates.md
index 08cc899..aeb21c1 100644
--- a/gcc/config/arm/predicates.md
+++ b/gcc/config/arm/predicates.md
@@ -117,6 +117,14 @@ (define_special_predicate "subreg_lowpart_operator"
   (and (match_code "subreg")
        (match_test "subreg_lowpart_p (op)")))
 
+(define_special_predicate "subreg_highpart_operator"
+  (and (match_code "subreg")
+       (match_test "GET_MODE_SIZE (mode)
+                      < GET_MODE_SIZE (GET_MODE (SUBREG_REG (op)))
+                    && SUBREG_BYTE (op)
+                        == subreg_highpart_offset (mode,
+                                                GET_MODE (SUBREG_REG (op)))")))
+
 ;; Reg, subreg(reg) or const_int.
 (define_predicate "reg_or_int_operand"
   (ior (match_code "const_int")

Re: [PATCH][RFA][LRA] Don't try to break down subreg expressions if insn already matches

[pinskia]

> On Feb 13, 2015, at 1:48 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> 
> Hi all,
> 
> In my tree added a pattern to the arm backend that's supposed to match:
> (set (reg:SI r0)
>     (subreg:SI
>       (plus:DI
>         (mult:DI (sign_extend:DI (reg:SI r1))
>                  (sign_extend:DI (reg:SI r2)))
>       (const_int 2147483648 [0x80000000])) 4))
> 
> That is, take two SImode regs, sign-extend to DImode, multiply in DImode,
> add a const_int and take the most significant SImode subreg.

Seems better to use shifts for the most significant simode and low part subreg after that. Isn't that what other targets do?

Thanks,
Andrew


> Combine matches it fine but I get an ICE in lra:
> 0xa665a5 crash_signal
>        $SRC/gcc/toplev.c:383
> 0x91ec1c lra_emit_add(rtx_def*, rtx_def*, rtx_def*)
>        $SRC/gcc/lra.c:418
> 0x91ef8a lra_emit_move(rtx_def*, rtx_def*)
>        $SRC/gcc/lra.c:528
> 0x9279b2 insert_move_for_subreg
>        $SRC/gcc/lra-constraints.c:1371
> 0x931bdb simplify_operand_subreg
>        $SRC/gcc/lra-constraints.c:1507
> 0x931bdb curr_insn_transform
>        $SRC/gcc/lra-constraints.c:3437
> 0x934793 lra_constraints(bool)
>        $SRC/gcc/lra-constraints.c:4451
> 0x9217d0 lra(_IO_FILE*)
>        $SRC/gcc/lra.c:2292
> 0x8e0dba do_reload
>        $SRC/gcc/ira.c:5418
> 0x8e0dba execute
>        $SRC/gcc/ira.c:5589
> 
> 
> I *think* the problem is that simplify_operand_subreg tries to break down
> this complex RTX involving the subreg into separate MULT/PLUS operations
> and breaking down the line.
> 
> If I apply the attached patch that stops trying to simplify the subreg
> expression if the whole thing matches something already my code compiles
> succesfully, and passes arm testing and x86_64 bootstrap.
> However, I'm concerned that calling recog_memoized is too heavy-handed,
> is there a better approach?
> Or is this even the right way to go about this?
> 
> I'm also attaching the patch to the arm backend that can be used to
> reproduce this ICE with the following C-code at -O2:
> 
> int
> smmulr (int a, int b)
> {
>  return ((long long)a * b + 0x80000000) >> 32;
> }
> 
> 
> Thanks,
> Kyrill
> 
> 2015-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
>    * lra-constraints.c (simplify_operand_subreg): Do not try to simplify
>    subreg if the pattern already matches.
> <lra-subreg.patch>
> <arm-smmulr.patch>

Re: [PATCH][RFA][LRA] Don't try to break down subreg expressions if insn already matches

[Kyrill Tkachov]

On 13/02/15 10:10, pinskia@gmail.com wrote:
>
>
>
>> On Feb 13, 2015, at 1:48 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>>
>> Hi all,
>>
>> In my tree added a pattern to the arm backend that's supposed to match:
>> (set (reg:SI r0)
>>      (subreg:SI
>>        (plus:DI
>>          (mult:DI (sign_extend:DI (reg:SI r1))
>>                   (sign_extend:DI (reg:SI r2)))
>>        (const_int 2147483648 [0x80000000])) 4))
>>
>> That is, take two SImode regs, sign-extend to DImode, multiply in DImode,
>> add a const_int and take the most significant SImode subreg.
> Seems better to use shifts for the most significant simode and low part subreg after that. Isn't that what other targets do?

I thought about that, but combine tries to match:
(set (reg/i:SI 0 r0)
     (subreg:SI (plus:DI (mult:DI (sign_extend:DI (reg:SI 0 r0 [ a ]))
                 (sign_extend:DI (reg:SI 1 r1 [ b ])))
             (const_int 2147483648 [0x80000000])) 4))


Looking at the RTL dumps all shifts are gone by the time combine is 
reached (in this case cse1 removes the shifts)

Kyrill

>
> Thanks,
> Andrew
>
>
>> Combine matches it fine but I get an ICE in lra:
>> 0xa665a5 crash_signal
>>         $SRC/gcc/toplev.c:383
>> 0x91ec1c lra_emit_add(rtx_def*, rtx_def*, rtx_def*)
>>         $SRC/gcc/lra.c:418
>> 0x91ef8a lra_emit_move(rtx_def*, rtx_def*)
>>         $SRC/gcc/lra.c:528
>> 0x9279b2 insert_move_for_subreg
>>         $SRC/gcc/lra-constraints.c:1371
>> 0x931bdb simplify_operand_subreg
>>         $SRC/gcc/lra-constraints.c:1507
>> 0x931bdb curr_insn_transform
>>         $SRC/gcc/lra-constraints.c:3437
>> 0x934793 lra_constraints(bool)
>>         $SRC/gcc/lra-constraints.c:4451
>> 0x9217d0 lra(_IO_FILE*)
>>         $SRC/gcc/lra.c:2292
>> 0x8e0dba do_reload
>>         $SRC/gcc/ira.c:5418
>> 0x8e0dba execute
>>         $SRC/gcc/ira.c:5589
>>
>>
>> I *think* the problem is that simplify_operand_subreg tries to break down
>> this complex RTX involving the subreg into separate MULT/PLUS operations
>> and breaking down the line.
>>
>> If I apply the attached patch that stops trying to simplify the subreg
>> expression if the whole thing matches something already my code compiles
>> succesfully, and passes arm testing and x86_64 bootstrap.
>> However, I'm concerned that calling recog_memoized is too heavy-handed,
>> is there a better approach?
>> Or is this even the right way to go about this?
>>
>> I'm also attaching the patch to the arm backend that can be used to
>> reproduce this ICE with the following C-code at -O2:
>>
>> int
>> smmulr (int a, int b)
>> {
>>   return ((long long)a * b + 0x80000000) >> 32;
>> }
>>
>>
>> Thanks,
>> Kyrill
>>
>> 2015-02-10  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>     * lra-constraints.c (simplify_operand_subreg): Do not try to simplify
>>     subreg if the pattern already matches.
>> <lra-subreg.patch>
>> <arm-smmulr.patch>

Re: [PATCH][RFA][LRA] Don't try to break down subreg expressions if insn already matches

[Maxim Kuvyrkov]

On Feb 13, 2015, at 1:47 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:

> 
> On 13/02/15 10:10, pinskia@gmail.com wrote:
>> 
>> 
>> 
>>> On Feb 13, 2015, at 1:48 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
>>> 
>>> Hi all,
>>> 
>>> In my tree added a pattern to the arm backend that's supposed to match:
>>> (set (reg:SI r0)
>>>     (subreg:SI
>>>       (plus:DI
>>>         (mult:DI (sign_extend:DI (reg:SI r1))
>>>                  (sign_extend:DI (reg:SI r2)))
>>>       (const_int 2147483648 [0x80000000])) 4))
>>> 
>>> That is, take two SImode regs, sign-extend to DImode, multiply in DImode,
>>> add a const_int and take the most significant SImode subreg.
>> Seems better to use shifts for the most significant simode and low part subreg after that. Isn't that what other targets do?
> 
> I thought about that, but combine tries to match:
> (set (reg/i:SI 0 r0)
>    (subreg:SI (plus:DI (mult:DI (sign_extend:DI (reg:SI 0 r0 [ a ]))
>                (sign_extend:DI (reg:SI 1 r1 [ b ])))
>            (const_int 2147483648 [0x80000000])) 4))
> 
> 
> Looking at the RTL dumps all shifts are gone by the time combine is reached (in this case cse1 removes the shifts)

FYI, (and not related to the core issue of this patch)

The use of mult vs shift by combine is a problem that Venkat is working on, see "[RFC] Tighten memory type assumption in RTL combiner pass" .  The combiner uses MULTs instead of SHIFTs for rtx'es that look like addresses, even when they are, in fact, mere logic/arithmetic operations.

--
Maxim Kuvyrkov
www.linaro.org

Re: [PATCH][RFA][LRA] Don't try to break down subreg expressions if insn already matches

[Jeff Law]

On 02/14/15 04:23, Maxim Kuvyrkov wrote:
>
> FYI, (and not related to the core issue of this patch)
>
> The use of mult vs shift by combine is a problem that Venkat is
> working on, see "[RFC] Tighten memory type assumption in RTL combiner
> pass" .  The combiner uses MULTs instead of SHIFTs for rtx'es that
> look like addresses, even when they are, in fact, mere
> logic/arithmetic operations.
Right.  I think I put that into my gcc-6 queue because we don't have a 
regression that requires this problem be fixed.

If you've got a BZ that's regressing because of this issue, don't 
hesitate to point it out and I'll move the thread into my gcc-5 queue.

jeff

Re: [PATCH][RFA][LRA] Don't try to break down subreg expressions if insn already matches

[Kyrill Tkachov]

On 17/02/15 06:18, Jeff Law wrote:
> On 02/14/15 04:23, Maxim Kuvyrkov wrote:
>> FYI, (and not related to the core issue of this patch)
>>
>> The use of mult vs shift by combine is a problem that Venkat is
>> working on, see "[RFC] Tighten memory type assumption in RTL combiner
>> pass" .  The combiner uses MULTs instead of SHIFTs for rtx'es that
>> look like addresses, even when they are, in fact, mere
>> logic/arithmetic operations.
> Right.  I think I put that into my gcc-6 queue because we don't have a
> regression that requires this problem be fixed.
>
> If you've got a BZ that's regressing because of this issue, don't
> hesitate to point it out and I'll move the thread into my gcc-5 queue.

Right, so I think I'll wait for GCC-6 when this work is done to try 
implement these patterns with shifts.

This LRA patch breaks on aarch64 anyway.

Thanks,
Kyrill

>
> jeff
>