Re: [PATCH] middle-end, i386, v3: Pattern recognize add/subtract with carry [PR79173]

[Uros Bizjak <ubizjak@gmail.com>]

On Wed, Jun 14, 2023 at 4:00 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> On Wed, Jun 14, 2023 at 12:35:42PM +0000, Richard Biener wrote:
> > At this point two pages of code without a comment - can you introduce
> > some vertical spacing and comments as to what is matched now?  The
> > split out functions help somewhat but the code is far from obvious :/
> >
> > Maybe I'm confused by the loops and instead of those sth like
> >
> >  if (match_x_y_z (op0)
> >      || match_x_y_z (op1))
> >    ...
> >
> > would be easier to follow with the loop bodies split out?
> > Maybe put just put them in lambdas even?
> >
> > I guess you'll be around as long as myself so we can go with
> > this code under the premise you're going to maintain it - it's
> > not that I'm writing trivially to understand code myself ...
>
> As I said on IRC, I don't really know how to split that into further
> functions, the problem is that we need to pattern match a lot of
> statements and it is hard to come up with names for each of them.
> And we need quite a lot of variables for checking their interactions.
>
> The code isn't that much different from say match_arith_overflow or
> optimize_spaceship or other larger pattern recognizers.  And the
> intent is that all the code paths in the recognizer are actually covered
> by the testcases in the testsuite.
>
> That said, I've added 18 new comments to the function, and rebased it
> on top of the
> https://gcc.gnu.org/pipermail/gcc-patches/2023-June/621717.html
> patch with all constant arguments handling moved to fold-const-call.cc
> even for the new ifns.
>
> Ok for trunk like this if it passes bootstrap/regtest?
>
> 2023-06-13  Jakub Jelinek  <jakub@redhat.com>
>
>         PR middle-end/79173
>         * internal-fn.def (UADDC, USUBC): New internal functions.
>         * internal-fn.cc (expand_UADDC, expand_USUBC): New functions.
>         (commutative_ternary_fn_p): Return true also for IFN_UADDC.
>         * optabs.def (uaddc5_optab, usubc5_optab): New optabs.
>         * tree-ssa-math-opts.cc (uaddc_cast, uaddc_ne0, uaddc_is_cplxpart,
>         match_uaddc_usubc): New functions.
>         (math_opts_dom_walker::after_dom_children): Call match_uaddc_usubc
>         for PLUS_EXPR, MINUS_EXPR, BIT_IOR_EXPR and BIT_XOR_EXPR unless
>         other optimizations have been successful for those.
>         * gimple-fold.cc (gimple_fold_call): Handle IFN_UADDC and IFN_USUBC.
>         * fold-const-call.cc (fold_const_call): Likewise.
>         * gimple-range-fold.cc (adjust_imagpart_expr): Likewise.
>         * tree-ssa-dce.cc (eliminate_unnecessary_stmts): Likewise.
>         * doc/md.texi (uaddc<mode>5, usubc<mode>5): Document new named
>         patterns.
>         * config/i386/i386.md (subborrow<mode>): Add alternative with
>         memory destination.
>         (uaddc<mode>5, usubc<mode>5): New define_expand patterns.
>         (*sub<mode>_3, @add<mode>3_carry, addcarry<mode>, @sub<mode>3_carry,
>         subborrow<mode>, *add<mode>3_cc_overflow_1): Add define_peephole2
>         TARGET_READ_MODIFY_WRITE/-Os patterns to prefer using memory
>         destination in these patterns.
>
>         * gcc.target/i386/pr79173-1.c: New test.
>         * gcc.target/i386/pr79173-2.c: New test.
>         * gcc.target/i386/pr79173-3.c: New test.
>         * gcc.target/i386/pr79173-4.c: New test.
>         * gcc.target/i386/pr79173-5.c: New test.
>         * gcc.target/i386/pr79173-6.c: New test.
>         * gcc.target/i386/pr79173-7.c: New test.
>         * gcc.target/i386/pr79173-8.c: New test.
>         * gcc.target/i386/pr79173-9.c: New test.
>         * gcc.target/i386/pr79173-10.c: New test.

+;; Helper peephole2 for the addcarry<mode> and subborrow<mode>
+;; peephole2s, to optimize away nop which resulted from uaddc/usubc
+;; expansion optimization.
+(define_peephole2
+  [(set (match_operand:SWI48 0 "general_reg_operand")
+       (match_operand:SWI48 1 "memory_operand"))
+   (const_int 0)]
+  ""
+  [(set (match_dup 0) (match_dup 1))])

Is this (const_int 0) from a recent patch from Roger that introduced:

+;; Set the carry flag from the carry flag.
+(define_insn_and_split "*setccc"
+  [(set (reg:CCC FLAGS_REG)
+ (reg:CCC FLAGS_REG))]
+  "ix86_pre_reload_split ()"
+  "#"
+  "&& 1"
+  [(const_int 0)])
+
+;; Set the carry flag from the carry flag.
+(define_insn_and_split "*setcc_qi_negqi_ccc_1_<mode>"
+  [(set (reg:CCC FLAGS_REG)
+ (ltu:CCC (reg:CC_CCC FLAGS_REG) (const_int 0)))]
+  "ix86_pre_reload_split ()"
+  "#"
+  "&& 1"
+  [(const_int 0)])
+
+;; Set the carry flag from the carry flag.
+(define_insn_and_split "*setcc_qi_negqi_ccc_2_<mode>"
+  [(set (reg:CCC FLAGS_REG)
+ (unspec:CCC [(ltu:QI (reg:CC_CCC FLAGS_REG) (const_int 0))
+     (const_int 0)] UNSPEC_CC_NE))]
+  "ix86_pre_reload_split ()"
+  "#"
+  "&& 1"
+  [(const_int 0)])

If this interferes with RTL stream, then instead of emitting
(const_int 0), the above patterns should simply emit:

{
  emit_note (NOTE_INSN_DELETED);
  DONE;
}

And there will be no (const_int 0) in the RTL stream.

Uros.