Turn most genrecog warnings into errors

genrecog does some useful sanity checks on the .md files.  At the moment
it only reports most of the problems as warnings though, which means you
won't notice them unless you specifically look.

I think the only message in validate_pattern that deserves to be a
warning is the one about missing modes, since the current code does
warn about valid cases.  It would be good to tighten that up at some
point, but not today.

Tested by building cc1 with and without the warning for each backend
(picking an arbitrary configuration triple in each case).  The patch
below updates backends for which the fix was truly obvious:
removing constraints from things that don't allow constraints,
replacing one target-independent predicate with another that
doesn't accept immediates, or using match_operand rather than
match_test to test subpredicates.  I've posted separate patches
for the backends that need changing in other ways.

Also tested in the normal way on x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
	* genrecog.c (validate_pattern): Treat all messages except missing
	modes as errors.
	* config/epiphany/epiphany.md: Remove constraints from
	define_peephole2s.
	* config/h8300/h8300.md: Remove constraints from define_splits.
	* config/msp430/msp430.md: Likewise.
	* config/mcore/mcore.md (movdi_i, movsf_i, movdf_k): Use
	nonimmediate_operand rather than general_operand for operand 0.
	* config/moxie/moxie.md (*movsi, *movqi, *movhi): Likewise.
	* config/pdp11/predicates.md (float_operand, float_nonimm_operand):
	Use match_operator rather than match_test to invoke general_operand.
	* config/v850/v850.md (*movqi_internal, *movhi_internal)
	(*movsi_internal_v850e, *movsi_internal, *movsf_internal): Likewise.

On Fri, Nov 22, 2013 at 9:43 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> genrecog does some useful sanity checks on the .md files.  At the moment
> it only reports most of the problems as warnings though, which means you
> won't notice them unless you specifically look.
>
> I think the only message in validate_pattern that deserves to be a
> warning is the one about missing modes, since the current code does
> warn about valid cases.  It would be good to tighten that up at some
> point, but not today.
>
> Tested by building cc1 with and without the warning for each backend
> (picking an arbitrary configuration triple in each case).  The patch
> below updates backends for which the fix was truly obvious:
> removing constraints from things that don't allow constraints,
> replacing one target-independent predicate with another that
> doesn't accept immediates, or using match_operand rather than
> match_test to test subpredicates.  I've posted separate patches
> for the backends that need changing in other ways.
>
> Also tested in the normal way on x86_64-linux-gnu.  OK to install?

Ok.

Thanks,
Richard.

> Thanks,
> Richard
>
>
> gcc/
>         * genrecog.c (validate_pattern): Treat all messages except missing
>         modes as errors.
>         * config/epiphany/epiphany.md: Remove constraints from
>         define_peephole2s.
>         * config/h8300/h8300.md: Remove constraints from define_splits.
>         * config/msp430/msp430.md: Likewise.
>         * config/mcore/mcore.md (movdi_i, movsf_i, movdf_k): Use
>         nonimmediate_operand rather than general_operand for operand 0.
>         * config/moxie/moxie.md (*movsi, *movqi, *movhi): Likewise.
>         * config/pdp11/predicates.md (float_operand, float_nonimm_operand):
>         Use match_operator rather than match_test to invoke general_operand.
>         * config/v850/v850.md (*movqi_internal, *movhi_internal)
>         (*movsi_internal_v850e, *movsi_internal, *movsf_internal): Likewise.
>
> Index: gcc/genrecog.c
> ===================================================================
> --- gcc/genrecog.c      2013-11-16 21:52:15.083787117 +0000
> +++ gcc/genrecog.c      2013-11-16 21:59:11.174140732 +0000
> @@ -457,9 +457,8 @@ validate_pattern (rtx pattern, rtx insn,
>           {
>             pred = lookup_predicate (pred_name);
>             if (!pred)
> -             message_with_line (pattern_lineno,
> -                                "warning: unknown predicate '%s'",
> -                                pred_name);
> +             error_with_line (pattern_lineno, "unknown predicate '%s'",
> +                              pred_name);
>           }
>         else
>           pred = 0;
> @@ -477,9 +476,9 @@ validate_pattern (rtx pattern, rtx insn,
>                 || GET_CODE (insn) == DEFINE_PEEPHOLE2)
>               {
>                 if (constraints0)
> -                 message_with_line (pattern_lineno,
> -                                    "warning: constraints not supported in %s",
> -                                    rtx_name[GET_CODE (insn)]);
> +                 error_with_line (pattern_lineno,
> +                                  "constraints not supported in %s",
> +                                  rtx_name[GET_CODE (insn)]);
>               }
>
>             /* A MATCH_OPERAND that is a SET should have an output reload.  */
> @@ -510,10 +509,9 @@ validate_pattern (rtx pattern, rtx insn,
>            while not likely to occur at runtime, results in less efficient
>            code from insn-recog.c.  */
>         if (set && pred && pred->allows_non_lvalue)
> -         message_with_line (pattern_lineno,
> -                            "warning: destination operand %d "
> -                            "allows non-lvalue",
> -                            XINT (pattern, 0));
> +         error_with_line (pattern_lineno,
> +                          "destination operand %d allows non-lvalue",
> +                          XINT (pattern, 0));
>
>         /* A modeless MATCH_OPERAND can be handy when we can check for
>            multiple modes in the c_test.  In most other cases, it is a
> @@ -781,16 +779,16 @@ add_to_sequence (rtx pattern, struct dec
>                 allows_const_int = pred->codes[CONST_INT];
>                 if (was_code == MATCH_PARALLEL
>                     && pred->singleton != PARALLEL)
> -                 message_with_line (pattern_lineno,
> -                       "predicate '%s' used in match_parallel "
> -                       "does not allow only PARALLEL", pred->name);
> +                 error_with_line (pattern_lineno,
> +                                  "predicate '%s' used in match_parallel "
> +                                  "does not allow only PARALLEL", pred->name);
>                 else
>                   code = pred->singleton;
>               }
>             else
> -             message_with_line (pattern_lineno,
> -                       "warning: unknown predicate '%s' in '%s' expression",
> -                       pred_name, GET_RTX_NAME (was_code));
> +             error_with_line (pattern_lineno,
> +                              "unknown predicate '%s' in '%s' expression",
> +                              pred_name, GET_RTX_NAME (was_code));
>           }
>
>         /* Can't enforce a mode if we allow const_int.  */
> Index: gcc/config/epiphany/epiphany.md
> ===================================================================
> --- gcc/config/epiphany/epiphany.md     2013-11-16 21:52:15.083787117 +0000
> +++ gcc/config/epiphany/epiphany.md     2013-11-16 21:59:11.174140732 +0000
> @@ -1787,14 +1787,14 @@ (define_insn_and_split "*mov_f"
>
>  (define_peephole2
>    [(parallel
> -    [(set (match_operand:SI 0 "gpr_operand" "=r")
> -         (logical_op:SI (match_operand:SI 1 "gpr_operand"  "r")
> -                        (match_operand:SI 2 "gpr_operand" "%r")))
> +    [(set (match_operand:SI 0 "gpr_operand")
> +         (logical_op:SI (match_operand:SI 1 "gpr_operand")
> +                        (match_operand:SI 2 "gpr_operand")))
>       (clobber (reg:CC CC_REGNUM))])
>     (parallel
>      [(set (reg:CC CC_REGNUM)
>           (compare:CC (and:SI (match_dup 0) (match_dup 0)) (const_int 0)))
> -     (set (match_operand:SI 3 "gpr_operand" "=r")
> +     (set (match_operand:SI 3 "gpr_operand")
>           (and:SI (match_dup 0) (match_dup 0)))])]
>    "peep2_reg_dead_p (2, operands[0])"
>    [(parallel
> @@ -1805,14 +1805,14 @@ (define_peephole2
>
>  (define_peephole2
>    [(parallel
> -    [(set (match_operand:SI 0 "gpr_operand" "=r")
> -         (logical_op:SI (match_operand:SI 1 "gpr_operand"  "r")
> -                        (match_operand:SI 2 "gpr_operand" "%r")))
> +    [(set (match_operand:SI 0 "gpr_operand")
> +         (logical_op:SI (match_operand:SI 1 "gpr_operand")
> +                        (match_operand:SI 2 "gpr_operand")))
>       (clobber (reg:CC CC_REGNUM))])
>     (parallel
>      [(set (reg:CC CC_REGNUM)
>           (compare:CC (and:SI (match_dup 0) (match_dup 0)) (const_int 0)))
> -     (set (match_operand:SI 3 "gpr_operand" "=r")
> +     (set (match_operand:SI 3 "gpr_operand")
>           (and:SI (match_dup 0) (match_dup 0)))])]
>    "peep2_reg_dead_p (2, operands[3])"
>    [(parallel
> @@ -1823,14 +1823,14 @@ (define_peephole2
>
>  (define_peephole2
>    [(parallel
> -    [(set (match_operand:SI 0 "gpr_operand" "=r")
> -         (logical_op:SI (match_operand:SI 1 "gpr_operand"  "r")
> -                        (match_operand:SI 2 "gpr_operand" "%r")))
> +    [(set (match_operand:SI 0 "gpr_operand")
> +         (logical_op:SI (match_operand:SI 1 "gpr_operand")
> +                        (match_operand:SI 2 "gpr_operand")))
>       (clobber (reg:CC CC_REGNUM))])
>     (parallel
>      [(set (reg:CC CC_REGNUM)
>           (compare:CC (match_dup 0) (const_int 0)))
> -     (clobber (match_operand:SI 3 "gpr_operand" "=r"))])]
> +     (clobber (match_operand:SI 3 "gpr_operand"))])]
>    ""
>    [(parallel
>      [(set (reg:CC CC_REGNUM)
> Index: gcc/config/h8300/h8300.md
> ===================================================================
> --- gcc/config/h8300/h8300.md   2013-11-16 21:52:15.083787117 +0000
> +++ gcc/config/h8300/h8300.md   2013-11-16 21:59:11.175140740 +0000
> @@ -1703,9 +1703,9 @@ (define_insn "bclrqi_msx"
>    [(set_attr "length" "8")])
>
>  (define_split
> -  [(set (match_operand:HI 0 "bit_register_indirect_operand" "=U")
> -       (and:HI (match_operand:HI 1 "bit_register_indirect_operand" "%0")
> -               (match_operand:HI 2 "single_zero_operand" "Y0")))]
> +  [(set (match_operand:HI 0 "bit_register_indirect_operand")
> +       (and:HI (match_operand:HI 1 "bit_register_indirect_operand")
> +               (match_operand:HI 2 "single_zero_operand")))]
>    "TARGET_H8300SX"
>    [(set (match_dup 0)
>         (and:QI (match_dup 1)
> @@ -1844,9 +1844,9 @@ (define_insn "bsetqi_msx"
>    [(set_attr "length" "8")])
>
>  (define_split
> -  [(set (match_operand:HI 0 "bit_register_indirect_operand" "=U")
> -       (ior:HI (match_operand:HI 1 "bit_register_indirect_operand" "%0")
> -               (match_operand:HI 2 "single_one_operand" "Y2")))]
> +  [(set (match_operand:HI 0 "bit_register_indirect_operand")
> +       (ior:HI (match_operand:HI 1 "bit_register_indirect_operand")
> +               (match_operand:HI 2 "single_one_operand")))]
>    "TARGET_H8300SX"
>    [(set (match_dup 0)
>         (ior:QI (match_dup 1)
> @@ -1922,9 +1922,9 @@ (define_insn "bnotqi_msx"
>    [(set_attr "length" "8")])
>
>  (define_split
> -  [(set (match_operand:HI 0 "bit_register_indirect_operand" "=U")
> -       (xor:HI (match_operand:HI 1 "bit_register_indirect_operand" "%0")
> -               (match_operand:HI 2 "single_one_operand" "Y2")))]
> +  [(set (match_operand:HI 0 "bit_register_indirect_operand")
> +       (xor:HI (match_operand:HI 1 "bit_register_indirect_operand")
> +               (match_operand:HI 2 "single_one_operand")))]
>    "TARGET_H8300SX"
>    [(set (match_dup 0)
>         (xor:QI (match_dup 1)
> Index: gcc/config/msp430/msp430.md
> ===================================================================
> --- gcc/config/msp430/msp430.md 2013-11-16 21:52:15.083787117 +0000
> +++ gcc/config/msp430/msp430.md 2013-11-16 21:59:11.177140757 +0000
> @@ -362,9 +362,9 @@ (define_insn "addchi4_cy"
>  ; so that gcc knows when it can and can't optimize away the two
>  ; halves.
>  (define_split
> -  [(set (match_operand:SI 0 "msp430_nonsubreg_operand" "=&rm")
> -       (plus:SI (match_operand:SI 1 "nonimmediate_operand" "%0")
> -                (match_operand:SI 2 "general_operand" "rmi")))
> +  [(set (match_operand:SI 0 "msp430_nonsubreg_operand")
> +       (plus:SI (match_operand:SI 1 "nonimmediate_operand")
> +                (match_operand:SI 2 "general_operand")))
>     ]
>    ""
>    [(parallel [(set (match_operand:HI 3 "nonimmediate_operand" "=&rm")
> Index: gcc/config/mcore/mcore.md
> ===================================================================
> --- gcc/config/mcore/mcore.md   2013-11-16 21:52:15.083787117 +0000
> +++ gcc/config/mcore/mcore.md   2013-11-16 21:59:11.176140748 +0000
> @@ -1288,7 +1288,7 @@ (define_expand "movdi"
>  }")
>
>  (define_insn "movdi_i"
> -  [(set (match_operand:DI 0 "general_operand" "=r,r,r,r,a,r,m")
> +  [(set (match_operand:DI 0 "nonimmediate_operand" "=r,r,r,r,a,r,m")
>         (match_operand:DI 1 "mcore_general_movsrc_operand" "I,M,N,r,R,m,r"))]
>    ""
>    "* return mcore_output_movedouble (operands, DImode);"
> @@ -1307,7 +1307,7 @@ (define_expand "movsf"
>  }")
>
>  (define_insn "movsf_i"
> -  [(set (match_operand:SF 0 "general_operand" "=r,r,m")
> +  [(set (match_operand:SF 0 "nonimmediate_operand" "=r,r,m")
>         (match_operand:SF 1 "general_operand"  "r,m,r"))]
>    ""
>    "@
> @@ -1329,7 +1329,7 @@ (define_expand "movdf"
>  }")
>
>  (define_insn "movdf_k"
> -  [(set (match_operand:DF 0 "general_operand" "=r,r,m")
> +  [(set (match_operand:DF 0 "nonimmediate_operand" "=r,r,m")
>         (match_operand:DF 1 "general_operand" "r,m,r"))]
>    ""
>    "* return mcore_output_movedouble (operands, DFmode);"
> Index: gcc/config/moxie/moxie.md
> ===================================================================
> --- gcc/config/moxie/moxie.md   2013-11-16 21:52:15.083787117 +0000
> +++ gcc/config/moxie/moxie.md   2013-11-16 21:59:11.176140748 +0000
> @@ -223,7 +223,7 @@ (define_expand "movsi"
>  }")
>
>  (define_insn "*movsi"
> -  [(set (match_operand:SI 0 "general_operand" "=r,r,r,W,A,r,r,B,r")
> +  [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,W,A,r,r,B,r")
>         (match_operand:SI 1 "moxie_general_movsrc_operand" "O,r,i,r,r,W,A,r,B"))]
>    "register_operand (operands[0], SImode)
>     || register_operand (operands[1], SImode)"
> @@ -251,7 +251,7 @@ (define_expand "movqi"
>  }")
>
>  (define_insn "*movqi"
> -  [(set (match_operand:QI 0 "general_operand" "=r,r,r,W,A,r,r,B,r")
> +  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,r,W,A,r,r,B,r")
>         (match_operand:QI 1 "moxie_general_movsrc_operand" "O,r,i,r,r,W,A,r,B"))]
>    "register_operand (operands[0], QImode)
>     || register_operand (operands[1], QImode)"
> @@ -279,7 +279,7 @@ (define_expand "movhi"
>  }")
>
>  (define_insn "*movhi"
> -  [(set (match_operand:HI 0 "general_operand" "=r,r,r,W,A,r,r,B,r")
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,W,A,r,r,B,r")
>         (match_operand:HI 1 "moxie_general_movsrc_operand" "O,r,i,r,r,W,A,r,B"))]
>    "(register_operand (operands[0], HImode)
>      || register_operand (operands[1], HImode))"
> Index: gcc/config/pdp11/predicates.md
> ===================================================================
> --- gcc/config/pdp11/predicates.md      2013-11-16 21:52:15.083787117 +0000
> +++ gcc/config/pdp11/predicates.md      2013-11-16 21:59:11.177140757 +0000
> @@ -42,7 +42,7 @@ (define_predicate "float_operand"
>                 (ior
>                  (match_test "REGNO_REG_CLASS (REGNO (op)) == LOAD_FPU_REGS")
>                  (match_test "REGNO_REG_CLASS (REGNO (op)) == NO_LOAD_FPU_REGS"))
> -               (match_test "general_operand (op, mode)")))
> +               (match_operand 0 "general_operand")))
>
>  ;; Accept anything nonimmediate_operand accepts, except that registers must
>  ;; be FPU registers.
> @@ -51,4 +51,4 @@ (define_predicate "float_nonimm_operand"
>                 (ior
>                  (match_test "REGNO_REG_CLASS (REGNO (op)) == LOAD_FPU_REGS")
>                  (match_test "REGNO_REG_CLASS (REGNO (op)) == NO_LOAD_FPU_REGS"))
> -               (match_test "nonimmediate_operand (op, mode)")))
> +               (match_operand 0 "nonimmediate_operand")))
> Index: gcc/config/v850/v850.md
> ===================================================================
> --- gcc/config/v850/v850.md     2013-11-16 21:52:15.083787117 +0000
> +++ gcc/config/v850/v850.md     2013-11-16 21:59:11.177140757 +0000
> @@ -233,7 +233,7 @@ (define_expand "movqi"
>    })
>
>  (define_insn "*movqi_internal"
> -  [(set (match_operand:QI 0 "general_operand" "=r,r,r,Q,r,m,m")
> +  [(set (match_operand:QI 0 "nonimmediate_operand" "=r,r,r,Q,r,m,m")
>         (match_operand:QI 1 "general_operand" "Jr,n,Q,Ir,m,r,I"))]
>    "register_operand (operands[0], QImode)
>     || reg_or_0_operand (operands[1], QImode)"
> @@ -258,7 +258,7 @@ (define_expand "movhi"
>  })
>
>  (define_insn "*movhi_internal"
> -  [(set (match_operand:HI 0 "general_operand" "=r,r,r,Q,r,m,m")
> +  [(set (match_operand:HI 0 "nonimmediate_operand" "=r,r,r,Q,r,m,m")
>         (match_operand:HI 1 "general_operand" "Jr,n,Q,Ir,m,r,I"))]
>    "register_operand (operands[0], HImode)
>     || reg_or_0_operand (operands[1], HImode)"
> @@ -334,7 +334,7 @@ (define_expand "movsi"
>  ;; upper part with hi, and then put the lower part in the load/store insn.
>
>  (define_insn "*movsi_internal_v850e"
> -  [(set (match_operand:SI 0 "general_operand" "=r,r,r,r,Q,r,r,m,m,r")
> +  [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,r,Q,r,r,m,m,r")
>         (match_operand:SI 1 "general_operand" "Jr,K,L,Q,Ir,m,R,r,I,i"))]
>    "(TARGET_V850E_UP)
>     && (register_operand (operands[0], SImode)
> @@ -347,7 +347,7 @@ (define_insn "*movsi_internal_v850e"
>     (set_attr "type" "other,other,other,load,other,load,other,store,store,other")])
>
>  (define_insn "*movsi_internal"
> -  [(set (match_operand:SI 0 "general_operand" "=r,r,r,r,Q,r,r,m,m")
> +  [(set (match_operand:SI 0 "nonimmediate_operand" "=r,r,r,r,Q,r,r,m,m")
>         (match_operand:SI 1 "movsi_source_operand" "Jr,K,L,Q,Ir,m,R,r,I"))]
>    "register_operand (operands[0], SImode)
>     || reg_or_0_operand (operands[1], SImode)"
> @@ -359,7 +359,7 @@ (define_insn "*movsi_internal"
>     (set_attr "type" "other,other,other,load,other,load,store,store,other")])
>
>  (define_insn "*movsf_internal"
> -  [(set (match_operand:SF 0 "general_operand" "=r,r,r,r,r,Q,r,m,m,r")
> +  [(set (match_operand:SF 0 "nonimmediate_operand" "=r,r,r,r,r,Q,r,m,m,r")
>         (match_operand:SF 1 "general_operand" "Jr,K,L,n,Q,Ir,m,r,IG,iF"))]
>    "register_operand (operands[0], SFmode)
>     || reg_or_0_operand (operands[1], SFmode)"

On Nov 22, 2013, at 3:43 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote:

> genrecog does some useful sanity checks on the .md files.  At the moment
> it only reports most of the problems as warnings though, which means you
> won't notice them unless you specifically look.
> 
> I think the only message in validate_pattern that deserves to be a
> warning is the one about missing modes, since the current code does
> warn about valid cases.  It would be good to tighten that up at some
> point, but not today.
> 
> Tested by building cc1 with and without the warning for each backend
> (picking an arbitrary configuration triple in each case).  The patch
> below updates backends for which the fix was truly obvious:
> removing constraints from things that don't allow constraints,
> replacing one target-independent predicate with another that
> doesn't accept immediates, or using match_operand rather than
> match_test to test subpredicates.  I've posted separate patches
> for the backends that need changing in other ways.
> 
> Also tested in the normal way on x86_64-linux-gnu.  OK to install?
> 
> Thanks,
> Richard
> 
> 
> gcc/
> 	* genrecog.c (validate_pattern): Treat all messages except missing
> 	modes as errors.
> 	* config/epiphany/epiphany.md: Remove constraints from
> 	define_peephole2s.
> 	* config/h8300/h8300.md: Remove constraints from define_splits.
> 	* config/msp430/msp430.md: Likewise.
> 	* config/mcore/mcore.md (movdi_i, movsf_i, movdf_k): Use
> 	nonimmediate_operand rather than general_operand for operand 0.
> 	* config/moxie/moxie.md (*movsi, *movqi, *movhi): Likewise.
> 	* config/pdp11/predicates.md (float_operand, float_nonimm_operand):
> 	Use match_operator rather than match_test to invoke general_operand.
> 	* config/v850/v850.md (*movqi_internal, *movhi_internal)
> 	(*movsi_internal_v850e, *movsi_internal, *movsf_internal): Likewise.

I think you meant "use match_operand rather than ..." (in config/pdp11/predicates.md).

	paul