[PATCH] i386: Remove unnecessary clobbers from combine splitters.

[PATCH] i386: Remove unnecessary clobbers from combine splitters.

[Uros Bizjak]

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

There is no need for combine splitters to emit insn patterns with clobbers,
the pass is smart enough to add clobbers to patterns as necessary.

2020-12-30  Uroš Bizjak  <ubizjak@gmail.com>

gcc/
    * config/i386/i386.md: Remove unnecessary clobbers
    from combine splitters.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

Pushed to the mainline.

Uros.

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

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index d7cd3df995c..ea1a0706dcb 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -12693,12 +12693,10 @@
 	  [(not:SWI (match_operand:SWI 2 "register_operand"))
 	   (match_operand:SWI 3 "nonimmediate_operand")]))]
   ""
-  [(parallel
-     [(set (reg:CCC FLAGS_REG)
-	   (compare:CCC
-	     (plus:SWI (match_dup 2) (match_dup 3))
-	     (match_dup 2)))
-      (clobber (scratch:SWI))])
+  [(set (reg:CCC FLAGS_REG)
+	(compare:CCC
+	  (plus:SWI (match_dup 2) (match_dup 3))
+	  (match_dup 2)))
    (set (match_dup 0)
 	(match_op_dup 1 [(reg:CCC FLAGS_REG) (const_int 0)]))])
 
@@ -12709,12 +12707,10 @@
 	   (match_operand 3 "const_int_operand")]))]
   "TARGET_64BIT
    && IN_RANGE (exact_log2 (UINTVAL (operands[3]) + 1), 32, 63)"
-  [(parallel
-     [(set (reg:CCZ FLAGS_REG)
-	   (compare:CCZ
-	     (lshiftrt:DI (match_dup 2) (match_dup 4))
-	     (const_int 0)))
-      (clobber (scratch:DI))])
+  [(set (reg:CCZ FLAGS_REG)
+	(compare:CCZ
+	  (lshiftrt:DI (match_dup 2) (match_dup 4))
+	  (const_int 0)))
    (set (match_dup 0)
 	(match_op_dup 1 [(reg:CCZ FLAGS_REG) (const_int 0)]))]
 {
@@ -12905,12 +12901,10 @@
 	  (label_ref (match_operand 0))
 	  (pc)))]
   ""
-  [(parallel
-     [(set (reg:CCC FLAGS_REG)
-	   (compare:CCC
-	     (plus:SWI (match_dup 2) (match_dup 3))
-	     (match_dup 2)))
-      (clobber (scratch:SWI))])
+  [(set (reg:CCC FLAGS_REG)
+	(compare:CCC
+	  (plus:SWI (match_dup 2) (match_dup 3))
+	  (match_dup 2)))
    (set (pc)
 	(if_then_else (match_op_dup 1 [(reg:CCC FLAGS_REG) (const_int 0)])
 		      (label_ref (match_operand 0))
@@ -12926,12 +12920,10 @@
 	  (pc)))]
   "TARGET_64BIT
    && IN_RANGE (exact_log2 (UINTVAL (operands[3]) + 1), 32, 63)"
-  [(parallel
-     [(set (reg:CCZ FLAGS_REG)
-	   (compare:CCZ
-	     (lshiftrt:DI (match_dup 2) (match_dup 4))
-	     (const_int 0)))
-      (clobber (scratch:DI))])
+  [(set (reg:CCZ FLAGS_REG)
+	(compare:CCZ
+	  (lshiftrt:DI (match_dup 2) (match_dup 4))
+	  (const_int 0)))
    (set (pc)
 	(if_then_else (match_op_dup 1 [(reg:CCZ FLAGS_REG) (const_int 0)])
 		      (label_ref (match_operand 0))
@@ -18581,9 +18573,8 @@
    && INTVAL (operands[2]) != -1
    && INTVAL (operands[2]) != 2147483647"
   [(set (reg:CC FLAGS_REG) (compare:CC (match_dup 1) (match_dup 2)))
-   (parallel [(set (match_dup 0)
-		   (neg:SWI48 (ltu:SWI48 (reg:CC FLAGS_REG) (const_int 0))))
-	      (clobber (reg:CC FLAGS_REG))])]
+   (set (match_dup 0)
+	(neg:SWI48 (ltu:SWI48 (reg:CC FLAGS_REG) (const_int 0))))]
   "operands[2] = GEN_INT (INTVAL (operands[2]) + 1);")
 
 (define_split
@@ -18594,9 +18585,8 @@
 	    (const_int 0))))]
   ""
   [(set (reg:CC FLAGS_REG) (compare:CC (match_dup 1) (const_int 1)))
-   (parallel [(set (match_dup 0)
-		   (neg:SWI (ltu:SWI (reg:CC FLAGS_REG) (const_int 0))))
-	      (clobber (reg:CC FLAGS_REG))])])
+   (set (match_dup 0)
+	(neg:SWI (ltu:SWI (reg:CC FLAGS_REG) (const_int 0))))])
 
 (define_split
   [(set (match_operand:SWI 0 "register_operand")
@@ -18605,13 +18595,10 @@
 	    (match_operand 1 "int_nonimmediate_operand")
 	    (const_int 0))))]
   ""
-  [(parallel [(set (reg:CCC FLAGS_REG)
-		   (ne:CCC (match_dup 1) (const_int 0)))
-	      (clobber (match_dup 2))])
-   (parallel [(set (match_dup 0)
-		   (neg:SWI (ltu:SWI (reg:CCC FLAGS_REG) (const_int 0))))
-	      (clobber (reg:CC FLAGS_REG))])]
-  "operands[2] = gen_rtx_SCRATCH (GET_MODE (operands[1]));")
+  [(set (reg:CCC FLAGS_REG)
+	(ne:CCC (match_dup 1) (const_int 0)))
+   (set (match_dup 0)
+	(neg:SWI (ltu:SWI (reg:CCC FLAGS_REG) (const_int 0))))])
 
 (define_insn "*mov<mode>cc_noc"
   [(set (match_operand:SWI248 0 "register_operand" "=r,r")

Re: [PATCH] i386: Remove unnecessary clobbers from combine splitters.

[Segher Boessenkool]

Hi Uros,

On Wed, Dec 30, 2020 at 05:44:50PM +0100, Uros Bizjak via Gcc-patches wrote:
> There is no need for combine splitters to emit insn patterns with clobbers,
> the pass is smart enough to add clobbers to patterns as necessary.

Nice.  Just one thing: in principle the splitters can be used outside of
combine, too?  This can lead to insns that do not recog() then?  Is
there anything that prevents that from happening?


Segher

Re: [PATCH] i386: Remove unnecessary clobbers from combine splitters.

[Uros Bizjak]

On Thu, Dec 31, 2020 at 9:40 AM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Hi Uros,
>
> On Wed, Dec 30, 2020 at 05:44:50PM +0100, Uros Bizjak via Gcc-patches wrote:
> > There is no need for combine splitters to emit insn patterns with clobbers,
> > the pass is smart enough to add clobbers to patterns as necessary.
>
> Nice.  Just one thing: in principle the splitters can be used outside of
> combine, too?  This can lead to insns that do not recog() then?  Is
> there anything that prevents that from happening?

No, combine splitters can't be used outside combine pass. These
splitters only split non-recognizable (non-existing) instructions, and
this is how they are told apart from general split insns. Also, most
of these combine splitters were added recently, but for those I added,
I was not aware of the clobber adding detail (which simplifies some
patterns quite nicely).

Uros.

Re: [PATCH] i386: Remove unnecessary clobbers from combine splitters.

[Segher Boessenkool]

Hi!

On Thu, Dec 31, 2020 at 09:54:01AM +0100, Uros Bizjak wrote:
> On Thu, Dec 31, 2020 at 9:40 AM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> > Nice.  Just one thing: in principle the splitters can be used outside of
> > combine, too?  This can lead to insns that do not recog() then?  Is
> > there anything that prevents that from happening?
> 
> No, combine splitters can't be used outside combine pass. These
> splitters only split non-recognizable (non-existing) instructions, and
> this is how they are told apart from general split insns.

There is only a define_split, not also a define_insn that matches the
pattern (or a define_insn_and_split), but that is *not* unique to
splitters that are meant for combine.

It isn't likely that any other pass would try to create this pattern,
but this isn't guaranteed, and such other passes do not necessarily do
the add-the-clobber (that is specific to combine, even!)  Maybe fwprop
could create this insn, or something like Richard's new combine-like
pass.

> Also, most
> of these combine splitters were added recently, but for those I added,
> I was not aware of the clobber adding detail (which simplifies some
> patterns quite nicely).

Indeed it does :-)  I just think it is a bit dangerous.


Segher

Re: [PATCH] i386: Remove unnecessary clobbers from combine splitters.

[Uros Bizjak]

On Thu, Dec 31, 2020 at 1:29 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> Hi!
>
> On Thu, Dec 31, 2020 at 09:54:01AM +0100, Uros Bizjak wrote:
> > On Thu, Dec 31, 2020 at 9:40 AM Segher Boessenkool
> > <segher@kernel.crashing.org> wrote:
> > > Nice.  Just one thing: in principle the splitters can be used outside of
> > > combine, too?  This can lead to insns that do not recog() then?  Is
> > > there anything that prevents that from happening?
> >
> > No, combine splitters can't be used outside combine pass. These
> > splitters only split non-recognizable (non-existing) instructions, and
> > this is how they are told apart from general split insns.
>
> There is only a define_split, not also a define_insn that matches the
> pattern (or a define_insn_and_split), but that is *not* unique to
> splitters that are meant for combine.
>
> It isn't likely that any other pass would try to create this pattern,
> but this isn't guaranteed, and such other passes do not necessarily do
> the add-the-clobber (that is specific to combine, even!)  Maybe fwprop
> could create this insn, or something like Richard's new combine-like
> pass.

I think that outside the combine pass, the insn should be recognized
first, so a classic define_insn_and_split would work, but not
define_split. IOW, other passes should only create valid insns. If
this is not the case, the behavior should be documented, and we'll fix
the splitters for this currently hypothetical problem.

Uros.

Re: [PATCH] i386: Remove unnecessary clobbers from combine splitters.

[Richard Sandiford]

Segher Boessenkool <segher@kernel.crashing.org> writes:
> It isn't likely that any other pass would try to create this pattern,
> but this isn't guaranteed, and such other passes do not necessarily do
> the add-the-clobber (that is specific to combine, even!)  Maybe fwprop
> could create this insn, or something like Richard's new combine-like
> pass.

FWIW, the rtl-ssa stuff also tries to add this kind of clobber where
necessary, meaning that fwprop now does too.  But that's only a comment
about those specific examples, not the general point.

In both cases, adding the clobber is part of recognising an instruction,
and can fail if the clobbered register is currently live.  Then of
course there's the decision about whether the split form is actually
a win.  So splits with clobbers would only conditionally succeed,
even in passes that know how to add them.

I agree with Uros that it seems unlikely that a pass would be allowed
to split one pattern that doesn't match without checking whether the
resulting instructions match, and without checking their cost.

Thanks,
Richard

Re: [PATCH] i386: Remove unnecessary clobbers from combine splitters.

[Segher Boessenkool]

On Thu, Dec 31, 2020 at 04:39:52PM +0000, Richard Sandiford wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > It isn't likely that any other pass would try to create this pattern,
> > but this isn't guaranteed, and such other passes do not necessarily do
> > the add-the-clobber (that is specific to combine, even!)  Maybe fwprop
> > could create this insn, or something like Richard's new combine-like
> > pass.
> 
> FWIW, the rtl-ssa stuff also tries to add this kind of clobber where
> necessary, meaning that fwprop now does too.  But that's only a comment
> about those specific examples, not the general point.
> 
> In both cases, adding the clobber is part of recognising an instruction,
> and can fail if the clobbered register is currently live.  Then of
> course there's the decision about whether the split form is actually
> a win.  So splits with clobbers would only conditionally succeed,
> even in passes that know how to add them.
> 
> I agree with Uros that it seems unlikely that a pass would be allowed
> to split one pattern that doesn't match without checking whether the
> resulting instructions match, and without checking their cost.

Ah, okay, a define_split without define_insn is only valid in combine:
md.texi:
  When the combiner phase tries to split an insn pattern, it is always the
  case that the pattern is @emph{not} matched by any @code{define_insn}.
  The combiner pass first tries to split a single @code{set} expression
  and then the same @code{set} expression inside a @code{parallel}, but
  followed by a @code{clobber} of a pseudo-reg to use as a scratch
  register.  In these cases, the combiner expects exactly one or two new insn
  patterns to be generated.  It will verify that these patterns match some
  @code{define_insn} definitions, so you need not do this test in the
  @code{define_split} (of course, there is no point in writing a
  @code{define_split} that will never produce insns that match).

So as long as people update the documentation whenever changing the
compiler, and your backend has no define_split that omits a clobber like
this for patterns that are also matched by a define_insn, all is good.

It is always hard to see if a splitter matches some other already
existing pattern (or whether a define_insn does, for that matter), but
that is a separate problem, much more general than this one.

Thanks for tolerating my worries,


Segher