On Mon, Feb 10, 2025 at 9:46 AM Jakub Jelinek <ja...@redhat.com> wrote:
>
> On Mon, Feb 10, 2025 at 12:26:03AM +0100, Uros Bizjak wrote:
> > On Sun, Feb 9, 2025 at 11:31 PM Jakub Jelinek <ja...@redhat.com> wrote:
> > >
> > > On Sun, Feb 09, 2025 at 09:24:30AM +0100, Uros Bizjak wrote:
> > > > "For commutative and comparison operators, a constant is always made
> > > > the second operand."
> > >
> > > Isn't that just for commutative comparison operators (eq, ne, ordered,
> > > unordered, ltgt, uneq)? Compare itself even isn't RTX_COMPARE at all,
> > > it is RTX_BIN_ARITH, so similar to minus.
> > > And I must say I can't find where simplify-rtx.cc or combine.cc would be
> > > trying to canonicalize that order (note, it would need to change the
> > > related
> > > uses of flags as well).
> >
> > When combine combines some RTX with any operand of COMPARE RTX, it
> > canonicalizes COMPARE RTX in simplify_compare_const:
> >
> > /* Try to simplify a comparison between OP0 and a constant OP1,
> > where CODE is the comparison code that will be tested, into a
> > (CODE OP0 const0_rtx) form.
> >
> > The result is a possibly different comparison code to use.
> > *POP0 and *POP1 may be updated. */
> >
> > This form won't match the new BT RTX.
> >
> > At the moment, there is no instruction using compare:CCC that could be
> > combined with e.g. ZERO_EXTRACT RTX to form "*bt<mode>" insn, so the
> > canonicalization rules do not matter, because BT is always expanded
> > from splitters in a kind-of manual way. This is the reason that patch
> > is OK, but without adding the new form to
> > ix86_canonicalize_comparison, combine won't be able to match the new
> > BT RTX in future.
> >
> > Please see ix86_canonicalize_comparison for a couple of examples on
> > how to allow combine to match non-canonical compares.
>
> So, would it be better to represent *bt<mode> as
> (set (reg:CCC FLAGS_REG)
> (compare:CCC (not:SWI48 (zero_extract:SWI48 ...)) (const_int -1)))
> rather than
> (set (reg:CCC FLAGS_REG)
> (compare:CCC (const_int 0) (zero_extract:SWI48 ...)))
> ?
> Or
> (set (reg:CCC FLAGS_REG)
> (compare:CCC (xor:SWI48 (zero_extract:SWI48 ...) (const_int 1))
> (const_int 1)))
> Though, simplify_compare_const has similarly code to transform
> (ltu something (const_int C)) into (leu something (const_int C-1))
> and that would affect both.
>
> Or do you want additionally:
> --- gcc/config/i386/i386.cc 2025-02-07 14:25:03.982866595 +0100
> +++ gcc/config/i386/i386.cc 2025-02-10 09:39:36.914272948 +0100
> @@ -595,6 +595,19 @@ ix86_canonicalize_comparison (int *code,
> *code = (int) swap_condition ((enum rtx_code) *code);
> return;
> }
> +
> + /* BT is represented as
> + (compare:CCC (const_int 0) (zero_extract ... (const_int 1) ...)) */
> + if (!op0_preserve_value
> + && (*code == GTU || *code == LEU)
> + && GET_CODE (*op0) == ZERO_EXTRACT
> + && XEXP (*op0, 1) == const1_rtx
> + && *op1 == const0_rtx)
> + {
> + std::swap (*op0, *op1);
> + *code = (int) swap_condition ((enum rtx_code) *code);
> + return;
> + }
> }
Adding a new rule to ix86_canonicalize_comparison is the way to go.
>
> /* Hook to determine if one function can safely inline another. */
> or so? But what will care to use CCCmode for this rather than some other?
True, ATM nothing uses plain CCCmode compare, so there are no
opportunities for combine. Based on that fact, the original patch is
OK, and adding additional canonicalization to
ix86_canonicalize_comparison can wait for some future time.
Uros.
