[PATCH, alpha]: Remove some_operand and some_ni_operand

[PATCH, alpha]: Remove some_operand and some_ni_operand

[Uros Bizjak]

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

We can use general_operand instead of some_operand.

2015-05-13  Uros Bizjak  <ubizjak@gmail.com>

    * config/alpha/alpha.md (extendqidi2): Use general_operand
    instead of some_operand for operand[1] predicate.
    (extendhidi2): Ditto.
    (cbranchdi4): Use general_operand instead of some_operand
    for operand[1] and operands[2] predicates.
    (cstoredi4): Ditto.
    * config/alpha/predicates.md (some_operand): Remove unused predicate.
    (some_ni_operand): Ditto.

Tested on alpha-linux-gnu.

Richard, does this look OK to you, or is there any other reason that
general_operand predicates were not used here?

Uros.

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

Index: config/alpha/alpha.md
===================================================================
--- config/alpha/alpha.md	(revision 223097)
+++ config/alpha/alpha.md	(working copy)
@@ -1235,7 +1235,7 @@
 
 (define_expand "extendqidi2"
   [(set (match_operand:DI 0 "register_operand")
-	(sign_extend:DI (match_operand:QI 1 "some_operand")))]
+	(sign_extend:DI (match_operand:QI 1 "general_operand")))]
   ""
 {
   if (TARGET_BWX)
@@ -1280,7 +1280,7 @@
 
 (define_expand "extendhidi2"
   [(set (match_operand:DI 0 "register_operand")
-	(sign_extend:DI (match_operand:HI 1 "some_operand")))]
+	(sign_extend:DI (match_operand:HI 1 "general_operand")))]
   ""
 {
   if (TARGET_BWX)
@@ -2902,8 +2902,8 @@
 
 (define_expand "cbranchdi4"
   [(use (match_operator 0 "alpha_cbranch_operator"
-         [(match_operand:DI 1 "some_operand")
-          (match_operand:DI 2 "some_operand")]))
+         [(match_operand:DI 1 "general_operand")
+          (match_operand:DI 2 "general_operand")]))
    (use (match_operand 3))]
   ""
   "alpha_emit_conditional_branch (operands, DImode); DONE;")
@@ -2936,8 +2936,8 @@
 
 (define_expand "cstoredi4"
   [(use (match_operator:DI 1 "alpha_cbranch_operator"
-         [(match_operand:DI 2 "some_operand")
-          (match_operand:DI 3 "some_operand")]))
+         [(match_operand:DI 2 "general_operand")
+          (match_operand:DI 3 "general_operand")]))
    (clobber (match_operand:DI 0 "register_operand"))]
   ""
 {
Index: config/alpha/predicates.md
===================================================================
--- config/alpha/predicates.md	(revision 223097)
+++ config/alpha/predicates.md	(working copy)
@@ -148,20 +148,6 @@
   return REGNO_REG_CLASS (REGNO (op)) == GENERAL_REGS;
 })
 
-;; Return 1 if OP is something that can be reloaded into a register;
-;; if it is a MEM, it need not be valid.
-(define_predicate "some_operand"
-  (ior (match_code "reg,mem,const_int,const_wide_int,const_double,const_vector,
-		    label_ref,symbol_ref,const,high")
-       (and (match_code "subreg")
-	    (match_test "some_operand (SUBREG_REG (op), VOIDmode)"))))
-
-;; Likewise, but don't accept constants.
-(define_predicate "some_ni_operand"
-  (ior (match_code "reg,mem")
-       (and (match_code "subreg")
-	    (match_test "some_ni_operand (SUBREG_REG (op), VOIDmode)"))))
-
 ;; Return 1 if OP is a valid operand for the source of a move insn.
 (define_predicate "input_operand"
   (match_code "label_ref,symbol_ref,const,high,reg,subreg,mem,

Re: [PATCH, alpha]: Remove some_operand and some_ni_operand

[Richard Henderson]

On 05/13/2015 11:11 AM, Uros Bizjak wrote:
> We can use general_operand instead of some_operand.
> 
> 2015-05-13  Uros Bizjak  <ubizjak@gmail.com>
> 
>     * config/alpha/alpha.md (extendqidi2): Use general_operand
>     instead of some_operand for operand[1] predicate.
>     (extendhidi2): Ditto.
>     (cbranchdi4): Use general_operand instead of some_operand
>     for operand[1] and operands[2] predicates.
>     (cstoredi4): Ditto.
>     * config/alpha/predicates.md (some_operand): Remove unused predicate.
>     (some_ni_operand): Ditto.
> 
> Tested on alpha-linux-gnu.
> 
> Richard, does this look OK to you, or is there any other reason that
> general_operand predicates were not used here?

For the extensions, it was put in by Kenner in 1997 (90f6b60d), to improve code
for unaligned memories.  That code was removed in 2011 by me (8b2983a3), so I
think dropping some_operand there is fine.

For the conditionals, it was added in 2004 by me (62350d6c), and that code is
still there.  Specifically,

@@ -3177,11 +3177,17 @@ alpha_emit_conditional_branch (enum rtx_code code)
 	    cmp_code = NIL, branch_code = code;

 	  /* If the constants doesn't fit into an immediate, but can
  	     be generated by lda/ldah, we adjust the argument and
  	     compare against zero, so we can use beq/bne directly.  */
-	  else if (GET_CODE (op1) == CONST_INT && (code == EQ || code == NE))
+	  /* ??? Don't do this when comparing against symbols, otherwise
+	     we'll reduce (&x == 0x1234) to (&x-0x1234 == 0), which will
+	     be declared false out of hand (at least for non-weak).  */
+	  else if (GET_CODE (op1) == CONST_INT
+		   && (code == EQ || code == NE)
+		   && !(symbolic_operand (op0, VOIDmode)
+			|| (GET_CODE (op0) == REG && REG_POINTER (op0))))

If I didn't use some_operand, the SYMBOL_REF would be lowered and we'll only
see a REG here.  Searching the mail archive I find

  https://gcc.gnu.org/ml/gcc-patches/2004-02/msg02436.html

pointing to the test case gcc.dg/20040123-1.c

Perhaps debugging that testcase to see what's reaching a_e_c_b in these modern
times will tell you what's most appropriate.


r~

Re: [PATCH, alpha]: Remove some_operand and some_ni_operand

[Uros Bizjak]

On Wed, May 13, 2015 at 8:53 PM, Richard Henderson <rth@redhat.com> wrote:
> On 05/13/2015 11:11 AM, Uros Bizjak wrote:
>> We can use general_operand instead of some_operand.
>>
>> 2015-05-13  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     * config/alpha/alpha.md (extendqidi2): Use general_operand
>>     instead of some_operand for operand[1] predicate.
>>     (extendhidi2): Ditto.
>>     (cbranchdi4): Use general_operand instead of some_operand
>>     for operand[1] and operands[2] predicates.
>>     (cstoredi4): Ditto.
>>     * config/alpha/predicates.md (some_operand): Remove unused predicate.
>>     (some_ni_operand): Ditto.
>>
>> Tested on alpha-linux-gnu.
>>
>> Richard, does this look OK to you, or is there any other reason that
>> general_operand predicates were not used here?
>
> For the extensions, it was put in by Kenner in 1997 (90f6b60d), to improve code
> for unaligned memories.  That code was removed in 2011 by me (8b2983a3), so I
> think dropping some_operand there is fine.
>
> For the conditionals, it was added in 2004 by me (62350d6c), and that code is
> still there.  Specifically,
>
> @@ -3177,11 +3177,17 @@ alpha_emit_conditional_branch (enum rtx_code code)
>             cmp_code = NIL, branch_code = code;
>
>           /* If the constants doesn't fit into an immediate, but can
>              be generated by lda/ldah, we adjust the argument and
>              compare against zero, so we can use beq/bne directly.  */
> -         else if (GET_CODE (op1) == CONST_INT && (code == EQ || code == NE))
> +         /* ??? Don't do this when comparing against symbols, otherwise
> +            we'll reduce (&x == 0x1234) to (&x-0x1234 == 0), which will
> +            be declared false out of hand (at least for non-weak).  */
> +         else if (GET_CODE (op1) == CONST_INT
> +                  && (code == EQ || code == NE)
> +                  && !(symbolic_operand (op0, VOIDmode)
> +                       || (GET_CODE (op0) == REG && REG_POINTER (op0))))
>
> If I didn't use some_operand, the SYMBOL_REF would be lowered and we'll only
> see a REG here.  Searching the mail archive I find
>
>   https://gcc.gnu.org/ml/gcc-patches/2004-02/msg02436.html
>
> pointing to the test case gcc.dg/20040123-1.c
>
> Perhaps debugging that testcase to see what's reaching a_e_c_b in these modern
> times will tell you what's most appropriate.

The reason for removing some_operand is, that it is effectively
exactly equal to general_operand:

from gensupport.c:

  {"general_operand", false, true, {SUBREG, REG, MEM}},

the second "true" stands for "bool allows_const_p", where:

  if (std_preds[i].allows_const_p)
    for (j = 0; j < NUM_RTX_CODE; j++)
      if (GET_RTX_CLASS (j) == RTX_CONST_OBJ)
        add_predicate_code (pred, (enum rtx_code) j);

and from rtl.def, these are all RTX_CONST_OBJs:

rtl.def:DEF_RTL_EXPR(CONST_INT, "const_int", "w", RTX_CONST_OBJ)
rtl.def:DEF_RTL_EXPR(CONST_WIDE_INT, "const_wide_int", "", RTX_CONST_OBJ)
rtl.def:DEF_RTL_EXPR(CONST_FIXED, "const_fixed", "www", RTX_CONST_OBJ)
rtl.def:DEF_RTL_EXPR(CONST_DOUBLE, "const_double",
CONST_DOUBLE_FORMAT, RTX_CONST_OBJ)
rtl.def:DEF_RTL_EXPR(CONST_VECTOR, "const_vector", "E", RTX_CONST_OBJ)
rtl.def:DEF_RTL_EXPR(CONST, "const", "e", RTX_CONST_OBJ)
rtl.def:DEF_RTL_EXPR(LABEL_REF, "label_ref", "u", RTX_CONST_OBJ)
rtl.def:DEF_RTL_EXPR(SYMBOL_REF, "symbol_ref", "s0", RTX_CONST_OBJ)
rtl.def:DEF_RTL_EXPR(HIGH, "high", "e", RTX_CONST_OBJ)

This is exactly the list of constant operands (minus const_fixed) from
some_operand predicate:

(define_predicate "some_operand"
  (ior (match_code "reg,mem,const_int,const_wide_int,const_double,const_vector,
                    label_ref,symbol_ref,const,high")
       (and (match_code "subreg")
            (match_test "some_operand (SUBREG_REG (op), VOIDmode)"))))

So, the only question is about SUBREG processing here, but I don't
think it matters here.

Uros.

Re: [PATCH, alpha]: Remove some_operand and some_ni_operand

[Uros Bizjak]

On Wed, May 13, 2015 at 8:53 PM, Richard Henderson <rth@redhat.com> wrote:
> On 05/13/2015 11:11 AM, Uros Bizjak wrote:
>> We can use general_operand instead of some_operand.
>>
>> 2015-05-13  Uros Bizjak  <ubizjak@gmail.com>
>>
>>     * config/alpha/alpha.md (extendqidi2): Use general_operand
>>     instead of some_operand for operand[1] predicate.
>>     (extendhidi2): Ditto.
>>     (cbranchdi4): Use general_operand instead of some_operand
>>     for operand[1] and operands[2] predicates.
>>     (cstoredi4): Ditto.
>>     * config/alpha/predicates.md (some_operand): Remove unused predicate.
>>     (some_ni_operand): Ditto.
>>
>> Tested on alpha-linux-gnu.
>>
>> Richard, does this look OK to you, or is there any other reason that
>> general_operand predicates were not used here?
>
> For the extensions, it was put in by Kenner in 1997 (90f6b60d), to improve code
> for unaligned memories.  That code was removed in 2011 by me (8b2983a3), so I
> think dropping some_operand there is fine.
>
> For the conditionals, it was added in 2004 by me (62350d6c), and that code is
> still there.  Specifically,
>
> @@ -3177,11 +3177,17 @@ alpha_emit_conditional_branch (enum rtx_code code)
>             cmp_code = NIL, branch_code = code;
>
>           /* If the constants doesn't fit into an immediate, but can
>              be generated by lda/ldah, we adjust the argument and
>              compare against zero, so we can use beq/bne directly.  */
> -         else if (GET_CODE (op1) == CONST_INT && (code == EQ || code == NE))
> +         /* ??? Don't do this when comparing against symbols, otherwise
> +            we'll reduce (&x == 0x1234) to (&x-0x1234 == 0), which will
> +            be declared false out of hand (at least for non-weak).  */
> +         else if (GET_CODE (op1) == CONST_INT
> +                  && (code == EQ || code == NE)
> +                  && !(symbolic_operand (op0, VOIDmode)
> +                       || (GET_CODE (op0) == REG && REG_POINTER (op0))))
>
> If I didn't use some_operand, the SYMBOL_REF would be lowered and we'll only
> see a REG here.  Searching the mail archive I find
>
>   https://gcc.gnu.org/ml/gcc-patches/2004-02/msg02436.html
>
> pointing to the test case gcc.dg/20040123-1.c
>
> Perhaps debugging that testcase to see what's reaching a_e_c_b in these modern
> times will tell you what's most appropriate.

Both, patched and unpatched compiler generate:

;; Generating RTL for gimple basic block 2

;; if (&a == 16384B)

(insn 5 4 6 (set (reg/f:DI 70)
        (symbol_ref:DI ("a") [flags 0x40]  <var_decl 0x200006f8360
a>)) 20040123-1.c:10 -1
     (nil))

(insn 6 5 7 (set (reg:DI 71)
        (const_int 16384 [0x4000])) 20040123-1.c:10 -1
     (nil))

(insn 7 6 8 (set (reg:DI 72)
        (eq:DI (reg/f:DI 70)
            (reg:DI 71))) 20040123-1.c:10 -1
     (nil))

(jump_insn 8 7 0 (set (pc)
        (if_then_else (eq (reg:DI 72)
                (const_int 0 [0]))
            (label_ref 0)
            (pc))) 20040123-1.c:10 -1
     (int_list:REG_BR_PROB 9996 (nil)))

and gcc.dg/20040123-1.c passes for as long as I remember...

Uros.

Re: [PATCH, alpha]: Remove some_operand and some_ni_operand

[Uros Bizjak]

On Thu, May 14, 2015 at 1:31 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Wed, May 13, 2015 at 8:53 PM, Richard Henderson <rth@redhat.com> wrote:
>> On 05/13/2015 11:11 AM, Uros Bizjak wrote:
>>> We can use general_operand instead of some_operand.
>>>
>>> 2015-05-13  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>     * config/alpha/alpha.md (extendqidi2): Use general_operand
>>>     instead of some_operand for operand[1] predicate.
>>>     (extendhidi2): Ditto.
>>>     (cbranchdi4): Use general_operand instead of some_operand
>>>     for operand[1] and operands[2] predicates.
>>>     (cstoredi4): Ditto.
>>>     * config/alpha/predicates.md (some_operand): Remove unused predicate.
>>>     (some_ni_operand): Ditto.
>>>
>>> Tested on alpha-linux-gnu.
>>>
>>> Richard, does this look OK to you, or is there any other reason that
>>> general_operand predicates were not used here?
>>
>> For the extensions, it was put in by Kenner in 1997 (90f6b60d), to improve code
>> for unaligned memories.  That code was removed in 2011 by me (8b2983a3), so I
>> think dropping some_operand there is fine.
>>
>> For the conditionals, it was added in 2004 by me (62350d6c), and that code is
>> still there.  Specifically,
>>
>> @@ -3177,11 +3177,17 @@ alpha_emit_conditional_branch (enum rtx_code code)
>>             cmp_code = NIL, branch_code = code;
>>
>>           /* If the constants doesn't fit into an immediate, but can
>>              be generated by lda/ldah, we adjust the argument and
>>              compare against zero, so we can use beq/bne directly.  */
>> -         else if (GET_CODE (op1) == CONST_INT && (code == EQ || code == NE))
>> +         /* ??? Don't do this when comparing against symbols, otherwise
>> +            we'll reduce (&x == 0x1234) to (&x-0x1234 == 0), which will
>> +            be declared false out of hand (at least for non-weak).  */
>> +         else if (GET_CODE (op1) == CONST_INT
>> +                  && (code == EQ || code == NE)
>> +                  && !(symbolic_operand (op0, VOIDmode)
>> +                       || (GET_CODE (op0) == REG && REG_POINTER (op0))))
>>
>> If I didn't use some_operand, the SYMBOL_REF would be lowered and we'll only
>> see a REG here.  Searching the mail archive I find
>>
>>   https://gcc.gnu.org/ml/gcc-patches/2004-02/msg02436.html
>>
>> pointing to the test case gcc.dg/20040123-1.c
>>
>> Perhaps debugging that testcase to see what's reaching a_e_c_b in these modern
>> times will tell you what's most appropriate.
>
> Both, patched and unpatched compiler generate:
>
> ;; Generating RTL for gimple basic block 2
>
> ;; if (&a == 16384B)
>
> (insn 5 4 6 (set (reg/f:DI 70)
>         (symbol_ref:DI ("a") [flags 0x40]  <var_decl 0x200006f8360
> a>)) 20040123-1.c:10 -1
>      (nil))
>
> (insn 6 5 7 (set (reg:DI 71)
>         (const_int 16384 [0x4000])) 20040123-1.c:10 -1
>      (nil))
>
> (insn 7 6 8 (set (reg:DI 72)
>         (eq:DI (reg/f:DI 70)
>             (reg:DI 71))) 20040123-1.c:10 -1
>      (nil))
>
> (jump_insn 8 7 0 (set (pc)
>         (if_then_else (eq (reg:DI 72)
>                 (const_int 0 [0]))
>             (label_ref 0)
>             (pc))) 20040123-1.c:10 -1
>      (int_list:REG_BR_PROB 9996 (nil)))
>
> and gcc.dg/20040123-1.c passes for as long as I remember...

Bah, pushed send too fast.

This is what is received by a_e_c_b in both, patched and unpatched case:

Breakpoint 1, alpha_emit_conditional_branch (operands=0x7fffffffd6e0,
cmp_mode=DImode) at
/home/uros/gcc-svn/trunk/gcc/config/alpha/alpha.c:2508
2508    alpha_emit_conditional_branch (rtx operands[], machine_mode cmp_mode)
(gdb) p debug_rtx (operands[0])
(ne (reg/f:DI 70)
    (const_int 16384 [0x4000]))
$1 = void
(gdb) p debug_rtx (operands[1])
(reg/f:DI 70)
$2 = void
(gdb) p debug_rtx (operands[2])
(const_int 16384 [0x4000])
$3 = void

Uros.

Re: [PATCH, alpha]: Remove some_operand and some_ni_operand

[Richard Henderson]

On 05/14/2015 04:40 AM, Uros Bizjak wrote:
> On Thu, May 14, 2015 at 1:31 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Wed, May 13, 2015 at 8:53 PM, Richard Henderson <rth@redhat.com> wrote:
>>> On 05/13/2015 11:11 AM, Uros Bizjak wrote:
>>>> We can use general_operand instead of some_operand.
>>>>
>>>> 2015-05-13  Uros Bizjak  <ubizjak@gmail.com>
>>>>
>>>>     * config/alpha/alpha.md (extendqidi2): Use general_operand
>>>>     instead of some_operand for operand[1] predicate.
>>>>     (extendhidi2): Ditto.
>>>>     (cbranchdi4): Use general_operand instead of some_operand
>>>>     for operand[1] and operands[2] predicates.
>>>>     (cstoredi4): Ditto.
>>>>     * config/alpha/predicates.md (some_operand): Remove unused predicate.
>>>>     (some_ni_operand): Ditto.
>>>>
>>>> Tested on alpha-linux-gnu.
>>>>
>>>> Richard, does this look OK to you, or is there any other reason that
>>>> general_operand predicates were not used here?
>>>
>>> For the extensions, it was put in by Kenner in 1997 (90f6b60d), to improve code
>>> for unaligned memories.  That code was removed in 2011 by me (8b2983a3), so I
>>> think dropping some_operand there is fine.
>>>
>>> For the conditionals, it was added in 2004 by me (62350d6c), and that code is
>>> still there.  Specifically,
>>>
>>> @@ -3177,11 +3177,17 @@ alpha_emit_conditional_branch (enum rtx_code code)
>>>             cmp_code = NIL, branch_code = code;
>>>
>>>           /* If the constants doesn't fit into an immediate, but can
>>>              be generated by lda/ldah, we adjust the argument and
>>>              compare against zero, so we can use beq/bne directly.  */
>>> -         else if (GET_CODE (op1) == CONST_INT && (code == EQ || code == NE))
>>> +         /* ??? Don't do this when comparing against symbols, otherwise
>>> +            we'll reduce (&x == 0x1234) to (&x-0x1234 == 0), which will
>>> +            be declared false out of hand (at least for non-weak).  */
>>> +         else if (GET_CODE (op1) == CONST_INT
>>> +                  && (code == EQ || code == NE)
>>> +                  && !(symbolic_operand (op0, VOIDmode)
>>> +                       || (GET_CODE (op0) == REG && REG_POINTER (op0))))
>>>
>>> If I didn't use some_operand, the SYMBOL_REF would be lowered and we'll only
>>> see a REG here.  Searching the mail archive I find
>>>
>>>   https://gcc.gnu.org/ml/gcc-patches/2004-02/msg02436.html
>>>
>>> pointing to the test case gcc.dg/20040123-1.c
>>>
>>> Perhaps debugging that testcase to see what's reaching a_e_c_b in these modern
>>> times will tell you what's most appropriate.
>>
>> Both, patched and unpatched compiler generate:
>>
>> ;; Generating RTL for gimple basic block 2
>>
>> ;; if (&a == 16384B)
>>
>> (insn 5 4 6 (set (reg/f:DI 70)
>>         (symbol_ref:DI ("a") [flags 0x40]  <var_decl 0x200006f8360
>> a>)) 20040123-1.c:10 -1
>>      (nil))
>>
>> (insn 6 5 7 (set (reg:DI 71)
>>         (const_int 16384 [0x4000])) 20040123-1.c:10 -1
>>      (nil))
>>
>> (insn 7 6 8 (set (reg:DI 72)
>>         (eq:DI (reg/f:DI 70)
>>             (reg:DI 71))) 20040123-1.c:10 -1
>>      (nil))
>>
>> (jump_insn 8 7 0 (set (pc)
>>         (if_then_else (eq (reg:DI 72)
>>                 (const_int 0 [0]))
>>             (label_ref 0)
>>             (pc))) 20040123-1.c:10 -1
>>      (int_list:REG_BR_PROB 9996 (nil)))
>>
>> and gcc.dg/20040123-1.c passes for as long as I remember...
> 
> Bah, pushed send too fast.
> 
> This is what is received by a_e_c_b in both, patched and unpatched case:
> 
> Breakpoint 1, alpha_emit_conditional_branch (operands=0x7fffffffd6e0,
> cmp_mode=DImode) at
> /home/uros/gcc-svn/trunk/gcc/config/alpha/alpha.c:2508
> 2508    alpha_emit_conditional_branch (rtx operands[], machine_mode cmp_mode)
> (gdb) p debug_rtx (operands[0])
> (ne (reg/f:DI 70)
>     (const_int 16384 [0x4000]))
> $1 = void
> (gdb) p debug_rtx (operands[1])
> (reg/f:DI 70)

Ok, gotcha -- the REG_POINTER_P section of the test is still triggering.  I
suspect that the symbolic_operand test can no longer trigger.

I think the whole patch is ok.


r~