[PATCH, doc] describe mode checking for doloop_end pattern

[PATCH, doc] describe mode checking for doloop_end pattern

[Paul Koning]

Since the code that uses the doloop_end pattern does not check the operand mode as given in the pattern, the pattern itself may need to do this, and that was not documented.  This patch adds that information.  It also updates the example to reflect this.

Ok for trunk?

	paul

ChangeLog:

2018-10-11  Paul Koning  <ni1d@arrl.net>

	* doc/md.texi (doloop_end): Document that the pattern code may
	need to check operand mode.

Index: doc/md.texi
===================================================================
--- doc/md.texi	(revision 265042)
+++ doc/md.texi	(working copy)
@@ -7619,7 +7619,23 @@ simplified) from the PDP-11 target:
 
 @smallexample
 @group
-(define_insn "doloop_end"
+(define_expand "doloop_end"
+  [(parallel [(set (pc)
+                   (if_then_else
+                    (ne (match_operand:HI 0 "nonimmediate_operand" "+r,!m")
+                        (const_int 1))
+                    (label_ref (match_operand 1 "" ""))
+                    (pc)))
+              (set (match_dup 0)
+                   (plus:HI (match_dup 0)
+                         (const_int -1)))])]
+  "TARGET_40_PLUS"
+  "@{
+    if (GET_MODE (operands[0]) != HImode)
+      FAIL;
+  @}")
+
+(define_insn "doloop_end_nocc"
   [(set (pc)
         (if_then_else
          (ne (match_operand:HI 0 "nonimmediate_operand" "+r,!m")
@@ -7628,17 +7644,28 @@ simplified) from the PDP-11 target:
          (pc)))
    (set (match_dup 0)
         (plus:HI (match_dup 0)
-              (const_int -1)))]
-  ""
-  
+              (const_int -1)))
+   (clobber (reg:CC CC_REGNUM))]
+  "TARGET_40_PLUS && reload_completed"
+  "*
   @{
+    rtx lb[1];
+   
     if (which_alternative == 0)
-      return "sob %0,%l1";
+       return \"sob\t%0,%l1\";
+   
+    /* emulate sob */
+    lb[0] = gen_label_rtx ();
+    output_asm_insn (\"dec\t%0\", operands);
+    output_asm_insn (\"beq\t%l0\", lb);
+    output_asm_insn (\"jmp\t%l1\", operands);
+    
+    output_asm_label (lb[0]);
+    fputs (\":\\n\", asm_out_file);
+   
+    return \"\";
+  @}")
 
-    /* emulate sob */
-    output_asm_insn ("dec %0", operands);
-    return "bne %l1";
-  @})
 @end group
 @end smallexample
 
@@ -7662,10 +7689,18 @@ will be non-negative.
 Since the @code{doloop_end} insn is a jump insn that also has an output,
 the reload pass does not handle the output operand.  Therefore, the
 constraint must allow for that operand to be in memory rather than a
-register.  In the example shown above, that is handled by using a loop
-instruction sequence that can handle memory operands when the memory
-alternative appears.
+register.  In the example shown above, that is handled (in the
+@code{doloop_end_nocc} pattern) by using a loop instruction sequence
+that can handle memory operands when the memory alternative appears.
 
+GCC does not check the mode of the loop register operand when generating
+the @code{doloop_end} pattern.  If the pattern is only valid for some
+modes but not others, the pattern should be a @code{define_expand}
+pattern that checks the operand mode in the preparation code, and issues
+@code{FAIL} if an unsupported mode is found.  The example above does
+this, since the machine instruction to be used only exists for
+@code{HImode}.
+
 @end ifset
 @ifset INTERNALS
 @node Insn Canonicalizations

Re: [PATCH, doc] describe mode checking for doloop_end pattern

[Sandra Loosemore]

On 10/11/2018 09:20 AM, Paul Koning wrote:
> Since the code that uses the doloop_end pattern does not check the operand mode as given in the pattern, the pattern itself may need to do this, and that was not documented.  This patch adds that information.  It also updates the example to reflect this.
> 
> Ok for trunk?

I have no comments on the technical correctness of this patch, but one 
nit-picky thing that caught my eye....

> Index: doc/md.texi
> ===================================================================
> --- doc/md.texi	(revision 265042)
> +++ doc/md.texi	(working copy)
> @@ -7619,7 +7619,23 @@ simplified) from the PDP-11 target:
>   
>   @smallexample
>   @group
> -(define_insn "doloop_end"
> +(define_expand "doloop_end"
> +  [(parallel [(set (pc)
> +                   (if_then_else
> +                    (ne (match_operand:HI 0 "nonimmediate_operand" "+r,!m")

I wonder if the above code line is too long and might overflow the right 
margin.  Did you try to generate a PDF and look at it?  Just to be safe, 
you might try to reformat this example to have less indentation.

Otherwise it looks OK to me.

-Sandra

[PATCH, doc] describe mode checking for doloop_end pattern

[Paul Koning]

Updated with an additional item I just debugged.

Since the code that uses the doloop_end pattern does not check the operand mode as given in the pattern, the pattern itself may need to do this, and that was not documented.  In addition, if the doloop_end pattern is a define_expand, there must be a define_insn (or define_insn_and_split) matching the generated pattern.  I had a define_split instead, and the result was an ICE in loop optimization (loop2_done pass).

This patch adds that information.  It also updates the example to reflect this.

Ok for trunk?

	paul

ChangeLog:

2018-10-11  Paul Koning  <ni1d@arrl.net>

	* doc/md.texi (doloop_end): Document that the pattern code may
	need to check operand mode.

Index: md.texi
===================================================================
--- md.texi	(revision 265042)
+++ md.texi	(working copy)
@@ -7619,7 +7619,23 @@ simplified) from the PDP-11 target:
 
 @smallexample
 @group
-(define_insn "doloop_end"
+(define_expand "doloop_end"
+  [(parallel [(set (pc)
+                   (if_then_else
+                    (ne (match_operand:HI 0 "nonimmediate_operand" "+r,!m")
+                        (const_int 1))
+                    (label_ref (match_operand 1 "" ""))
+                    (pc)))
+              (set (match_dup 0)
+                   (plus:HI (match_dup 0)
+                         (const_int -1)))])]
+  ""
+  "@{
+    if (GET_MODE (operands[0]) != HImode)
+      FAIL;
+  @}")
+
+(define_insn "doloop_end_insn"
   [(set (pc)
         (if_then_else
          (ne (match_operand:HI 0 "nonimmediate_operand" "+r,!m")
@@ -7662,10 +7678,23 @@ will be non-negative.
 Since the @code{doloop_end} insn is a jump insn that also has an output,
 the reload pass does not handle the output operand.  Therefore, the
 constraint must allow for that operand to be in memory rather than a
-register.  In the example shown above, that is handled by using a loop
-instruction sequence that can handle memory operands when the memory
-alternative appears.
+register.  In the example shown above, that is handled (in the
+@code{doloop_end_nocc} pattern) by using a loop instruction sequence
+that can handle memory operands when the memory alternative appears.
 
+GCC does not check the mode of the loop register operand when generating
+the @code{doloop_end} pattern.  If the pattern is only valid for some
+modes but not others, the pattern should be a @code{define_expand}
+pattern that checks the operand mode in the preparation code, and issues
+@code{FAIL} if an unsupported mode is found.  The example above does
+this, since the machine instruction to be used only exists for
+@code{HImode}.
+
+If the @code{doloop_end} pattern is a @code{define_expand}, there must
+also be a @code{define_insn} or @code{define_insn_and_split} matching
+the generated pattern.  Otherwise, the compiler will fail during loop
+optimization.
+
 @end ifset
 @ifset INTERNALS
 @node Insn Canonicalizations

Re: [PATCH, doc] describe mode checking for doloop_end pattern

[Jeff Law]

On 10/11/18 3:09 PM, Paul Koning wrote:
> Updated with an additional item I just debugged.
> 
> Since the code that uses the doloop_end pattern does not check the operand mode as given in the pattern, the pattern itself may need to do this, and that was not documented.  In addition, if the doloop_end pattern is a define_expand, there must be a define_insn (or define_insn_and_split) matching the generated pattern.  I had a define_split instead, and the result was an ICE in loop optimization (loop2_done pass).
> 
> This patch adds that information.  It also updates the example to reflect this.
> 
> Ok for trunk?
> 
> 	paul
> 
> ChangeLog:
> 
> 2018-10-11  Paul Koning  <ni1d@arrl.net>
> 
> 	* doc/md.texi (doloop_end): Document that the pattern code may
> 	need to check operand mode.
OK.
jeff

Re: [PATCH, doc] describe mode checking for doloop_end pattern

[Paul Koning]

> On Oct 11, 2018, at 11:01 PM, Jeff Law <law@redhat.com> wrote:
> 
> On 10/11/18 3:09 PM, Paul Koning wrote:
>> Updated with an additional item I just debugged.
>> 
>> Since the code that uses the doloop_end pattern does not check the operand mode as given in the pattern, the pattern itself may need to do this, and that was not documented.  In addition, if the doloop_end pattern is a define_expand, there must be a define_insn (or define_insn_and_split) matching the generated pattern.  I had a define_split instead, and the result was an ICE in loop optimization (loop2_done pass).
>> 
>> This patch adds that information.  It also updates the example to reflect this.
>> 
>> Ok for trunk?
>> 
>> 	paul
>> 
>> ChangeLog:
>> 
>> 2018-10-11  Paul Koning  <ni1d@arrl.net>
>> 
>> 	* doc/md.texi (doloop_end): Document that the pattern code may
>> 	need to check operand mode.
> OK.
> jeff

Thanks.  Committed.

	paul