Re: Canonicalization of compares performed as side-effect operations
On 14/08/2019 08:48, Eric Botcazou wrote: [Sorry for the delay, I missed your question...] Interesting. Does it work for the general case of a reverse subtract, which I need to handle as wel? Not clear, Visium only uses it for SNE and combined NEG/SNE. I worked through the logic. I'm pretty sure it will work for any unsigned or equality comparison and for any constant. The general transformation is (cst uns/eq/op reg) == (~reg uns/eq/op ~cst) R.
Re: Canonicalization of compares performed as side-effect operations
[Sorry for the delay, I missed your question...] > Interesting. Does it work for the general case of a reverse subtract, > which I need to handle as wel? Not clear, Visium only uses it for SNE and combined NEG/SNE. -- Eric Botcazou
Re: Canonicalization of compares performed as side-effect operations
On Tue, Aug 6, 2019 at 11:03 PM Richard Earnshaw (lists)
wrote:
>
> On 06/08/2019 18:39, Uros Bizjak wrote:
> > Hello!
> >
> >> On Tue, Aug 06, 2019 at 05:49:17PM +0100, Richard Earnshaw (lists) wrote:
> >>> On 06/08/2019 17:39, Segher Boessenkool wrote:
> >> What's wrong with describing the canonical form in your MD? You'll
> >> need
> >> some reversed condition code thingy, but that's it?
> >
> > It doesn't describe what the instruction does. The negation has a side
> > effect of setting the flags, but the flags are swapped because the
> > side-effect comparison is swapped from a normal compare. As I
> > mentioned, SELECT_CC_MODE doesn't help because it can't see the context
> > and the comparison just looks 'normal'.
>
> Sure, and we can work on making combine do what you want, but your
> existing
> pattern is *incorrect*. It needs fixing, and probably before we do other
> things.
> >>>
> >>> Why is it incorrect? It's not canonical, sure. But the cannonical form
> >>> does NOT describe what the instruction does.
> >>
> >> More precisely: not having the canonical form of this in your MD is what
> >> is incorrect.
> >
> > There is TARGET_CANONICALIZE_COMPARISON target hook that can solve the
> > issue here. Please see x86, where we canonicalize
> > *cmp__i387 via
> > ix86_canonicalize_comparison. As said in the comment:
> >
> > /* The order of operands in x87 ficom compare is forced by combine in
> > simplify_comparison () function. Float operator is treated as RTX_OBJ
> > with a precedence over other operators and is always put in the first
> > place. Swap condition and operands to match ficom instruction. */
> >
> > The issue looks similar to me.
> >
>
> That hook can't help as it can't see anything other than the operands of
> the compare, and from the operands it looks as though the operands
> should be swapped. To correctly canonicalize this you need the whole
> parallel, or some other hint that the operation is a side-effect of
> another operation.
I see. FYI, there is the same problem in i386.md with "*neg2_cmpz"
--cut here--
;; The problem with neg is that it does not perform (compare x 0),
;; it really performs (compare 0 x), which leaves us with the zero
;; flag being the only useful item.
(define_insn "*neg2_cmpz"
[(set (reg:CCZ FLAGS_REG)
(compare:CCZ
(neg:SWI (match_operand:SWI 1 "nonimmediate_operand" "0"))
(const_int 0)))
(set (match_operand:SWI 0 "nonimmediate_operand" "=m")
(neg:SWI (match_dup 1)))]
--cut here--
However, when trying to generate the pattern using following testcase:
--cut here--
void foo (void);
void bar (void);
int
test (int a, int b)
{
int r;
if (r = -b)
foo ();
else
bar ();
return r;
}
--cut here--
combine doesn't even consider the combination, since the compare looks
at the original value:
(insn 7 4 8 2 (parallel [
(set (reg/v:SI 82 [ ])
(neg:SI (reg/v:SI 84 [ b ])))
(clobber (reg:CC 17 flags))
]) "neg.c":9:9 461 {*negsi2_1}
(expr_list:REG_UNUSED (reg:CC 17 flags)
(nil)))
(insn 8 7 9 2 (set (reg:CCZ 17 flags)
(compare:CCZ (reg/v:SI 84 [ b ])
(const_int 0 [0]))) "neg.c":9:6 7 {*cmpsi_ccno_1}
(expr_list:REG_DEAD (reg/v:SI 84 [ b ])
(nil)))
probably due to tree optimizers that give us:
r_3 = -b_2(D);
if (b_2(D) != 0)
goto ; [50.00%]
else
goto ; [50.00%]
Uros.
Re: Canonicalization of compares performed as side-effect operations
On 06/08/2019 18:30, Eric Botcazou wrote: >> Why is it incorrect? It's not canonical, sure. But the cannonical form >> does NOT describe what the instruction does. > > Yes, you run into this when you try to be clever with the carry. For the > Visium port I kludged around it by using: > > [(set (reg:CCC R_FLAGS) > (compare:CCC (not:I (match_operand:I 0 "register_operand" "r")) >(const_int -1)))] > > and recognizing the -1 in SELECT_CC_MODE. Obviously cumbersome though. > Interesting. Does it work for the general case of a reverse subtract, which I need to handle as wel?
Re: Canonicalization of compares performed as side-effect operations
On 06/08/2019 18:39, Uros Bizjak wrote: > Hello! > >> On Tue, Aug 06, 2019 at 05:49:17PM +0100, Richard Earnshaw (lists) wrote: >>> On 06/08/2019 17:39, Segher Boessenkool wrote: >> What's wrong with describing the canonical form in your MD? You'll need >> some reversed condition code thingy, but that's it? > > It doesn't describe what the instruction does. The negation has a side > effect of setting the flags, but the flags are swapped because the > side-effect comparison is swapped from a normal compare. As I > mentioned, SELECT_CC_MODE doesn't help because it can't see the context > and the comparison just looks 'normal'. Sure, and we can work on making combine do what you want, but your existing pattern is *incorrect*. It needs fixing, and probably before we do other things. >>> >>> Why is it incorrect? It's not canonical, sure. But the cannonical form >>> does NOT describe what the instruction does. >> >> More precisely: not having the canonical form of this in your MD is what >> is incorrect. > > There is TARGET_CANONICALIZE_COMPARISON target hook that can solve the > issue here. Please see x86, where we canonicalize > *cmp__i387 via > ix86_canonicalize_comparison. As said in the comment: > > /* The order of operands in x87 ficom compare is forced by combine in > simplify_comparison () function. Float operator is treated as RTX_OBJ > with a precedence over other operators and is always put in the first > place. Swap condition and operands to match ficom instruction. */ > > The issue looks similar to me. > That hook can't help as it can't see anything other than the operands of the compare, and from the operands it looks as though the operands should be swapped. To correctly canonicalize this you need the whole parallel, or some other hint that the operation is a side-effect of another operation. R. > Uros. >
Re: Canonicalization of compares performed as side-effect operations
Hello! > On Tue, Aug 06, 2019 at 05:49:17PM +0100, Richard Earnshaw (lists) wrote: >> On 06/08/2019 17:39, Segher Boessenkool wrote: >> >>>What's wrong with describing the canonical form in your MD? You'll need >> >>>some reversed condition code thingy, but that's it? >> >> >> >>It doesn't describe what the instruction does. The negation has a side >> >>effect of setting the flags, but the flags are swapped because the >> >>side-effect comparison is swapped from a normal compare. As I >> >>mentioned, SELECT_CC_MODE doesn't help because it can't see the context >> >>and the comparison just looks 'normal'. >> > >> >Sure, and we can work on making combine do what you want, but your existing >> >pattern is *incorrect*. It needs fixing, and probably before we do other >> >things. >> >> Why is it incorrect? It's not canonical, sure. But the cannonical form >> does NOT describe what the instruction does. > > More precisely: not having the canonical form of this in your MD is what > is incorrect. There is TARGET_CANONICALIZE_COMPARISON target hook that can solve the issue here. Please see x86, where we canonicalize *cmp__i387 via ix86_canonicalize_comparison. As said in the comment: /* The order of operands in x87 ficom compare is forced by combine in simplify_comparison () function. Float operator is treated as RTX_OBJ with a precedence over other operators and is always put in the first place. Swap condition and operands to match ficom instruction. */ The issue looks similar to me. Uros.
Re: Canonicalization of compares performed as side-effect operations
> Why is it incorrect? It's not canonical, sure. But the cannonical form > does NOT describe what the instruction does. Yes, you run into this when you try to be clever with the carry. For the Visium port I kludged around it by using: [(set (reg:CCC R_FLAGS) (compare:CCC (not:I (match_operand:I 0 "register_operand" "r")) (const_int -1)))] and recognizing the -1 in SELECT_CC_MODE. Obviously cumbersome though. -- Eric Botcazou
Re: Canonicalization of compares performed as side-effect operations
On Tue, Aug 06, 2019 at 05:49:17PM +0100, Richard Earnshaw (lists) wrote: > On 06/08/2019 17:39, Segher Boessenkool wrote: > >>>What's wrong with describing the canonical form in your MD? You'll need > >>>some reversed condition code thingy, but that's it? > >> > >>It doesn't describe what the instruction does. The negation has a side > >>effect of setting the flags, but the flags are swapped because the > >>side-effect comparison is swapped from a normal compare. As I > >>mentioned, SELECT_CC_MODE doesn't help because it can't see the context > >>and the comparison just looks 'normal'. > > > >Sure, and we can work on making combine do what you want, but your existing > >pattern is *incorrect*. It needs fixing, and probably before we do other > >things. > > Why is it incorrect? It's not canonical, sure. But the cannonical form > does NOT describe what the instruction does. More precisely: not having the canonical form of this in your MD is what is incorrect. Segher
Re: Canonicalization of compares performed as side-effect operations
On 06/08/2019 17:39, Segher Boessenkool wrote: On Tue, Aug 06, 2019 at 05:22:48PM +0100, Richard Earnshaw (lists) wrote: On 06/08/2019 17:17, Segher Boessenkool wrote: Hi Richard, On Tue, Aug 06, 2019 at 04:35:04PM +0100, Richard Earnshaw (lists) wrote: Arm has an instruction that performs the following operation: (parallel [ (set (reg:CC 100 cc) (compare:CC (const_int 0 [0]) (reg:SI 121))) (set (reg:SI 113) (neg:SI (reg:SI 121))) ]) This is simply a reverse subtract from the constant zero, and setting the condition flags. It's the low part of a negdi2 expansion. However, combine will rip this up and try to transform the compare into 'canonical' form, ie (parallel [ (set (reg:CC 100 cc) (compare:CC (reg:SI 121) (const_int 0 [0]))) (set (reg:SI 113) (neg:SI (reg:SI 121))) ]) (and obviously swapping the condition on the instruction that uses the comparison result). This, of course, doesn't match the behaviour of the instruction and no-longer matches the pattern in the md file. It is, however, canonical RTL: (from md.texi:) In addition to algebraic simplifications, following canonicalizations are performed: @itemize @bullet @item For commutative and comparison operators, a constant is always made the second operand. If a machine only supports a constant as the second operand, only patterns that match a constant in the second operand need be supplied. Putting the constant first is non-canonical RTL and will in general not match any instructions generated by GCC. So is there a way to describe this instruction within the compiler, or a way to stop simplify_set from making this sort of simplification? What's wrong with describing the canonical form in your MD? You'll need some reversed condition code thingy, but that's it? It doesn't describe what the instruction does. The negation has a side effect of setting the flags, but the flags are swapped because the side-effect comparison is swapped from a normal compare. As I mentioned, SELECT_CC_MODE doesn't help because it can't see the context and the comparison just looks 'normal'. Sure, and we can work on making combine do what you want, but your existing pattern is *incorrect*. It needs fixing, and probably before we do other things. Why is it incorrect? It's not canonical, sure. But the cannonical form does NOT describe what the instruction does. R.
Re: Canonicalization of compares performed as side-effect operations
On Tue, Aug 06, 2019 at 05:22:48PM +0100, Richard Earnshaw (lists) wrote: > On 06/08/2019 17:17, Segher Boessenkool wrote: > >Hi Richard, > > > >On Tue, Aug 06, 2019 at 04:35:04PM +0100, Richard Earnshaw (lists) wrote: > >>Arm has an instruction that performs the following operation: > >> > >>(parallel [ > >> (set (reg:CC 100 cc) > >> (compare:CC (const_int 0 [0]) > >> (reg:SI 121))) > >> (set (reg:SI 113) > >> (neg:SI (reg:SI 121))) > >> ]) > >> > >>This is simply a reverse subtract from the constant zero, and setting > >>the condition flags. It's the low part of a negdi2 expansion. > >> > >>However, combine will rip this up and try to transform the compare into > >>'canonical' form, ie > >> > >>(parallel [ > >> (set (reg:CC 100 cc) > >> (compare:CC (reg:SI 121) > >> (const_int 0 [0]))) > >> (set (reg:SI 113) > >> (neg:SI (reg:SI 121))) > >> ]) > >> > >>(and obviously swapping the condition on the instruction that uses the > >>comparison result). > >> > >>This, of course, doesn't match the behaviour of the instruction and > >>no-longer matches the pattern in the md file. > > > >It is, however, canonical RTL: > > > >(from md.texi:) > > > > In addition to algebraic simplifications, following canonicalizations > > are performed: > > > > @itemize @bullet > > @item > > For commutative and comparison operators, a constant is always made the > > second operand. If a machine only supports a constant as the second > > operand, only patterns that match a constant in the second operand need > > be supplied. > > > >Putting the constant first is non-canonical RTL and will in general not > >match any instructions generated by GCC. > > > >>So is there a way to describe this instruction within the compiler, or a > >>way to stop simplify_set from making this sort of simplification? > > > >What's wrong with describing the canonical form in your MD? You'll need > >some reversed condition code thingy, but that's it? > > It doesn't describe what the instruction does. The negation has a side > effect of setting the flags, but the flags are swapped because the > side-effect comparison is swapped from a normal compare. As I > mentioned, SELECT_CC_MODE doesn't help because it can't see the context > and the comparison just looks 'normal'. Sure, and we can work on making combine do what you want, but your existing pattern is *incorrect*. It needs fixing, and probably before we do other things. Segher
Re: Canonicalization of compares performed as side-effect operations
On 06/08/2019 17:22, Richard Earnshaw (lists) wrote: On 06/08/2019 17:17, Segher Boessenkool wrote: Hi Richard, On Tue, Aug 06, 2019 at 04:35:04PM +0100, Richard Earnshaw (lists) wrote: Arm has an instruction that performs the following operation: (parallel [ (set (reg:CC 100 cc) (compare:CC (const_int 0 [0]) (reg:SI 121))) (set (reg:SI 113) (neg:SI (reg:SI 121))) ]) This is simply a reverse subtract from the constant zero, and setting the condition flags. It's the low part of a negdi2 expansion. However, combine will rip this up and try to transform the compare into 'canonical' form, ie (parallel [ (set (reg:CC 100 cc) (compare:CC (reg:SI 121) (const_int 0 [0]))) (set (reg:SI 113) (neg:SI (reg:SI 121))) ]) (and obviously swapping the condition on the instruction that uses the comparison result). This, of course, doesn't match the behaviour of the instruction and no-longer matches the pattern in the md file. It is, however, canonical RTL: (from md.texi:) In addition to algebraic simplifications, following canonicalizations are performed: @itemize @bullet @item For commutative and comparison operators, a constant is always made the second operand. If a machine only supports a constant as the second operand, only patterns that match a constant in the second operand need be supplied. Putting the constant first is non-canonical RTL and will in general not match any instructions generated by GCC. So is there a way to describe this instruction within the compiler, or a way to stop simplify_set from making this sort of simplification? What's wrong with describing the canonical form in your MD? You'll need some reversed condition code thingy, but that's it? It doesn't describe what the instruction does. The negation has a side effect of setting the flags, but the flags are swapped because the side-effect comparison is swapped from a normal compare. As I mentioned, SELECT_CC_MODE doesn't help because it can't see the context and the comparison just looks 'normal'. R. See, for example, this comment in combine.c: /* Many machines that don't use CC0 have insns that can both perform an arithmetic operation and set the condition code. These operations will be represented as a PARALLEL with the first element of the vector being a COMPARE of an arithmetic operation with the constant zero. The second element of the vector will set some pseudo to the result of the same arithmetic operation. If we simplify the COMPARE, we won't match such a pattern and so will generate an extra insn. Here we test for this case, where both the comparison and the operation result are needed, and make the PARALLEL by just replacing I2DEST in I3SRC with I2SRC. Later we will make the PARALLEL that contains I2. */ This is exactly the scenario we have here, but despite the comment, this is still happening in other places in combine.
Re: Canonicalization of compares performed as side-effect operations
On 06/08/2019 17:17, Segher Boessenkool wrote: Hi Richard, On Tue, Aug 06, 2019 at 04:35:04PM +0100, Richard Earnshaw (lists) wrote: Arm has an instruction that performs the following operation: (parallel [ (set (reg:CC 100 cc) (compare:CC (const_int 0 [0]) (reg:SI 121))) (set (reg:SI 113) (neg:SI (reg:SI 121))) ]) This is simply a reverse subtract from the constant zero, and setting the condition flags. It's the low part of a negdi2 expansion. However, combine will rip this up and try to transform the compare into 'canonical' form, ie (parallel [ (set (reg:CC 100 cc) (compare:CC (reg:SI 121) (const_int 0 [0]))) (set (reg:SI 113) (neg:SI (reg:SI 121))) ]) (and obviously swapping the condition on the instruction that uses the comparison result). This, of course, doesn't match the behaviour of the instruction and no-longer matches the pattern in the md file. It is, however, canonical RTL: (from md.texi:) In addition to algebraic simplifications, following canonicalizations are performed: @itemize @bullet @item For commutative and comparison operators, a constant is always made the second operand. If a machine only supports a constant as the second operand, only patterns that match a constant in the second operand need be supplied. Putting the constant first is non-canonical RTL and will in general not match any instructions generated by GCC. So is there a way to describe this instruction within the compiler, or a way to stop simplify_set from making this sort of simplification? What's wrong with describing the canonical form in your MD? You'll need some reversed condition code thingy, but that's it? It doesn't describe what the instruction does. The negation has a side effect of setting the flags, but the flags are swapped because the side-effect comparison is swapped from a normal compare. As I mentioned, SELECT_CC_MODE doesn't help because it can't see the context and the comparison just looks 'normal'. R.
Re: Canonicalization of compares performed as side-effect operations
Hi Richard, On Tue, Aug 06, 2019 at 04:35:04PM +0100, Richard Earnshaw (lists) wrote: > Arm has an instruction that performs the following operation: > > (parallel [ > (set (reg:CC 100 cc) > (compare:CC (const_int 0 [0]) > (reg:SI 121))) > (set (reg:SI 113) > (neg:SI (reg:SI 121))) > ]) > > This is simply a reverse subtract from the constant zero, and setting > the condition flags. It's the low part of a negdi2 expansion. > > However, combine will rip this up and try to transform the compare into > 'canonical' form, ie > > (parallel [ > (set (reg:CC 100 cc) > (compare:CC (reg:SI 121) > (const_int 0 [0]))) > (set (reg:SI 113) > (neg:SI (reg:SI 121))) > ]) > > (and obviously swapping the condition on the instruction that uses the > comparison result). > > This, of course, doesn't match the behaviour of the instruction and > no-longer matches the pattern in the md file. It is, however, canonical RTL: (from md.texi:) In addition to algebraic simplifications, following canonicalizations are performed: @itemize @bullet @item For commutative and comparison operators, a constant is always made the second operand. If a machine only supports a constant as the second operand, only patterns that match a constant in the second operand need be supplied. Putting the constant first is non-canonical RTL and will in general not match any instructions generated by GCC. > So is there a way to describe this instruction within the compiler, or a > way to stop simplify_set from making this sort of simplification? What's wrong with describing the canonical form in your MD? You'll need some reversed condition code thingy, but that's it? Segher
