Message ID | CAFULd4YAB9K2f3zMb8BYw=aU4L7J0sevjFfHsZhCFFr3d1iAdw@mail.gmail.com |
---|---|
State | New |
Headers | show |
Series | i386: Remove unnecessary clobbers from combine splitters. | expand |
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
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.
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
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.
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
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
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")