diff mbox series

[v1,1/2] Genmatch: Support new flow for phi on condition

Message ID 20240904072413.1282149-1-pan2.li@intel.com
State New
Headers show
Series [v1,1/2] Genmatch: Support new flow for phi on condition | expand

Commit Message

Li, Pan2 Sept. 4, 2024, 7:24 a.m. UTC
From: Pan Li <pan2.li@intel.com>

The gen_phi_on_cond can only support below control flow for cond
from day 1.  Aka:

+------+
| def  |
| ...  |       +-----+
| cond |------>| def |
+------+       | ... |
   |           +-----+
   |              |
   v              |
+-----+           |
| PHI |<----------+
+-----+

Unfortunately, there will be more scenarios of control flow on PHI.
For example as below:

T __attribute__((noinline))                            \
sat_s_add_##T##_fmt_3 (T x, T y)                       \
{                                                      \
  T sum;                                               \
  bool overflow = __builtin_add_overflow (x, y, &sum); \
  return overflow ? x < 0 ? MIN : MAX : sum;           \
}

DEF_SAT_S_ADD_FMT_3(int8_t, uint8_t, INT8_MIN, INT8_MAX)

With expanded RTL like below.
   3   │
   4   │ __attribute__((noinline))
   5   │ int8_t sat_s_add_int8_t_fmt_3 (int8_t x, int8_t y)
   6   │ {
   7   │   signed char _1;
   8   │   signed char _2;
   9   │   int8_t _3;
  10   │   __complex__ signed char _6;
  11   │   _Bool _8;
  12   │   signed char _9;
  13   │   signed char _10;
  14   │   signed char _11;
  15   │
  16   │ ;;   basic block 2, loop depth 0
  17   │ ;;    pred:       ENTRY
  18   │   _6 = .ADD_OVERFLOW (x_4(D), y_5(D));
  19   │   _2 = IMAGPART_EXPR <_6>;
  20   │   if (_2 != 0)
  21   │     goto <bb 4>; [50.00%]
  22   │   else
  23   │     goto <bb 3>; [50.00%]
  24   │ ;;    succ:       4
  25   │ ;;                3
  26   │
  27   │ ;;   basic block 3, loop depth 0
  28   │ ;;    pred:       2
  29   │   _1 = REALPART_EXPR <_6>;
  30   │   goto <bb 5>; [100.00%]
  31   │ ;;    succ:       5
  32   │
  33   │ ;;   basic block 4, loop depth 0
  34   │ ;;    pred:       2
  35   │   _8 = x_4(D) < 0;
  36   │   _9 = (signed char) _8;
  37   │   _10 = -_9;
  38   │   _11 = _10 ^ 127;
  39   │ ;;    succ:       5
  40   │
  41   │ ;;   basic block 5, loop depth 0
  42   │ ;;    pred:       3
  43   │ ;;                4
  44   │   # _3 = PHI <_1(3), _11(4)>
  45   │   return _3;
  46   │ ;;    succ:       EXIT
  47   │
  48   │ }

The above code will have below control flow which is not supported by
the gen_phi_on_cond.

+------+
| def  |
| ...  |       +-----+
| cond |------>| def |
+------+       | ... |
   |           +-----+
   |              |
   v              |
+-----+           |
| def |           |
| ... |           |
+-----+           |
   |              |
   |              |
   v              |
+-----+           |
| PHI |<----------+
+-----+

This patch would like to add support above control flow for the
gen_phi_on_cond.

The below testsuites are passed for this patch:
* The rv64gcv fully regression test.
* The x86 bootstrap test.
* The x86 fully regression test.

gcc/ChangeLog:

	* genmatch.cc (dt_operand::gen_phi_on_cond): Add support for
	a new control flow when gen phi on condition.

Signed-off-by: Pan Li <pan2.li@intel.com>
---
 gcc/genmatch.cc | 85 +++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 76 insertions(+), 9 deletions(-)

Comments

Richard Biener Sept. 4, 2024, 7:41 a.m. UTC | #1
On Wed, Sep 4, 2024 at 9:25 AM <pan2.li@intel.com> wrote:
>
> From: Pan Li <pan2.li@intel.com>
>
> The gen_phi_on_cond can only support below control flow for cond
> from day 1.  Aka:
>
> +------+
> | def  |
> | ...  |       +-----+
> | cond |------>| def |
> +------+       | ... |
>    |           +-----+
>    |              |
>    v              |
> +-----+           |
> | PHI |<----------+
> +-----+
>
> Unfortunately, there will be more scenarios of control flow on PHI.
> For example as below:
>
> T __attribute__((noinline))                            \
> sat_s_add_##T##_fmt_3 (T x, T y)                       \
> {                                                      \
>   T sum;                                               \
>   bool overflow = __builtin_add_overflow (x, y, &sum); \
>   return overflow ? x < 0 ? MIN : MAX : sum;           \
> }
>
> DEF_SAT_S_ADD_FMT_3(int8_t, uint8_t, INT8_MIN, INT8_MAX)
>
> With expanded RTL like below.
>    3   │
>    4   │ __attribute__((noinline))
>    5   │ int8_t sat_s_add_int8_t_fmt_3 (int8_t x, int8_t y)
>    6   │ {
>    7   │   signed char _1;
>    8   │   signed char _2;
>    9   │   int8_t _3;
>   10   │   __complex__ signed char _6;
>   11   │   _Bool _8;
>   12   │   signed char _9;
>   13   │   signed char _10;
>   14   │   signed char _11;
>   15   │
>   16   │ ;;   basic block 2, loop depth 0
>   17   │ ;;    pred:       ENTRY
>   18   │   _6 = .ADD_OVERFLOW (x_4(D), y_5(D));
>   19   │   _2 = IMAGPART_EXPR <_6>;
>   20   │   if (_2 != 0)
>   21   │     goto <bb 4>; [50.00%]
>   22   │   else
>   23   │     goto <bb 3>; [50.00%]
>   24   │ ;;    succ:       4
>   25   │ ;;                3
>   26   │
>   27   │ ;;   basic block 3, loop depth 0
>   28   │ ;;    pred:       2
>   29   │   _1 = REALPART_EXPR <_6>;
>   30   │   goto <bb 5>; [100.00%]
>   31   │ ;;    succ:       5
>   32   │
>   33   │ ;;   basic block 4, loop depth 0
>   34   │ ;;    pred:       2
>   35   │   _8 = x_4(D) < 0;
>   36   │   _9 = (signed char) _8;
>   37   │   _10 = -_9;
>   38   │   _11 = _10 ^ 127;
>   39   │ ;;    succ:       5
>   40   │
>   41   │ ;;   basic block 5, loop depth 0
>   42   │ ;;    pred:       3
>   43   │ ;;                4
>   44   │   # _3 = PHI <_1(3), _11(4)>
>   45   │   return _3;
>   46   │ ;;    succ:       EXIT
>   47   │
>   48   │ }
>
> The above code will have below control flow which is not supported by
> the gen_phi_on_cond.
>
> +------+
> | def  |
> | ...  |       +-----+
> | cond |------>| def |
> +------+       | ... |
>    |           +-----+
>    |              |
>    v              |
> +-----+           |
> | def |           |
> | ... |           |
> +-----+           |
>    |              |
>    |              |
>    v              |
> +-----+           |
> | PHI |<----------+
> +-----+
>
> This patch would like to add support above control flow for the
> gen_phi_on_cond.
>
> The below testsuites are passed for this patch:
> * The rv64gcv fully regression test.
> * The x86 bootstrap test.
> * The x86 fully regression test.

I'm lazy - can you please quote genmatch generated code for the condition for
one case?

Thanks,
Richard.

> gcc/ChangeLog:
>
>         * genmatch.cc (dt_operand::gen_phi_on_cond): Add support for
>         a new control flow when gen phi on condition.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
>  gcc/genmatch.cc | 85 +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 76 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/genmatch.cc b/gcc/genmatch.cc
> index a56bd90cb2c..f538df1be62 100644
> --- a/gcc/genmatch.cc
> +++ b/gcc/genmatch.cc
> @@ -3529,28 +3529,95 @@ dt_operand::gen_phi_on_cond (FILE *f, int indent, int depth)
>      "basic_block _pb_0_%d = EDGE_PRED (_b%d, 0)->src;\n", depth, depth);
>    fprintf_indent (f, indent,
>      "basic_block _pb_1_%d = EDGE_PRED (_b%d, 1)->src;\n", depth, depth);
> +
>    fprintf_indent (f, indent,
> -    "basic_block _db_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d)) ? "
> -    "_pb_0_%d : _pb_1_%d;\n", depth, depth, depth, depth);
> +    "gcond *_ct_0_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d));\n",
> +    depth, depth);
>    fprintf_indent (f, indent,
> -    "basic_block _other_db_%d = safe_dyn_cast <gcond *> "
> -    "(*gsi_last_bb (_pb_0_%d)) ? _pb_1_%d : _pb_0_%d;\n",
> +    "gcond *_ct_1_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_1_%d));\n",
> +    depth, depth);
> +  fprintf_indent (f, indent,
> +    "gcond *_ct_a_%d = _ct_0_%d ? _ct_0_%d : _ct_1_%d;\n",
> +    depth, depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "basic_block _db_%d = _ct_0_%d ? _pb_0_%d : _pb_1_%d;\n",
> +    depth, depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "basic_block _other_db_%d = _ct_0_%d ? _pb_1_%d : _pb_0_%d;\n",
>      depth, depth, depth, depth);
>
>    fprintf_indent (f, indent,
> -    "gcond *_ct_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_db_%d));\n",
> -    depth, depth);
> -  fprintf_indent (f, indent, "if (_ct_%d"
> +    "edge _e_00_%d = _pb_0_%d->preds ? EDGE_PRED (_pb_0_%d, 0) : NULL;\n",
> +    depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "basic_block _pb_00_%d = _e_00_%d ? _e_00_%d->src : NULL;\n",
> +    depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "gcond *_ct_b_%d = _pb_00_%d ? "
> +    "safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_00_%d)) : NULL;\n",
> +    depth, depth, depth);
> +
> +  /* Case 1 flow for PHI.
> +   * +------+
> +   * | def  |
> +   * | ...  |       +-----+
> +   * | cond |------>| def |
> +   * +------+       | ... |
> +   *    |           +-----+
> +   *    |              |
> +   *    v              |
> +   * +-----+           |
> +   * | PHI |<----------+
> +   * +-----+
> +   */
> +  fprintf_indent (f, indent, "if ((_ct_a_%d"
>      " && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth, depth);
>    fprintf_indent (f, indent,
> -    "  && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> +    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> +  fprintf_indent (f, indent,
> +    "     && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
> +
> +  fprintf_indent (f, indent, "    ||\n");
> +
> +  /* Case 2 flow for PHI.
> +   * +------+
> +   * | def  |
> +   * | ...  |       +-----+
> +   * | cond |------>| def |
> +   * +------+       | ... |
> +   *    |           +-----+
> +   *    |              |
> +   *    v              |
> +   * +-----+           |
> +   * | def |           |
> +   * | ... |           |
> +   * +-----+           |
> +   *    |              |
> +   *    |              |
> +   *    v              |
> +   * +-----+           |
> +   * | PHI |<----------+
> +   * +-----+
> +   */
> +  fprintf_indent (f, indent,
> +    "    (_ct_b_%d && _pb_00_%d && EDGE_COUNT (_pb_0_%d->succs) == 1\n",
> +    depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "     && EDGE_COUNT (_pb_0_%d->preds) == 1\n", depth);
>    fprintf_indent (f, indent,
> -    "  && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
> +    "     && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth);
> +  fprintf_indent (f, indent,
> +    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> +  fprintf_indent (f, indent,
> +    "     && EDGE_PRED (_other_db_%d, 0)->src == _pb_00_%d))\n", depth, depth);
>
>    indent += 2;
>    fprintf_indent (f, indent, "{\n");
>    indent += 2;
>
> +  fprintf_indent (f, indent,
> +    "gcond *_ct_%d = _ct_a_%d ? _ct_a_%d : _ct_b_%d;\n",
> +    depth, depth, depth, depth);
>    fprintf_indent (f, indent,
>      "tree _cond_lhs_%d = gimple_cond_lhs (_ct_%d);\n", depth, depth);
>    fprintf_indent (f, indent,
> --
> 2.43.0
>
Li, Pan2 Sept. 4, 2024, 7:48 a.m. UTC | #2
> I'm lazy - can you please quote genmatch generated code for the condition for
> one case?

Sure thing, list the before and after covers all the changes to generated code as blow.

Before this patch:
........      basic_block _b1 = gimple_bb (_a1);
........      if (gimple_phi_num_args (_a1) == 2)
................{
................  basic_block _pb_0_1 = EDGE_PRED (_b1, 0)->src;
................  basic_block _pb_1_1 = EDGE_PRED (_b1, 1)->src;
................  basic_block _db_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1)) ? _pb_0_1 : _pb_1_1;
................  basic_block _other_db_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1)) ? _pb_1_1 : _pb_0_1;
................  gcond *_ct_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_db_1));
................  if (_ct_1 && EDGE_COUNT (_other_db_1->preds) == 1
................    && EDGE_COUNT (_other_db_1->succs) == 1
................    && EDGE_PRED (_other_db_1, 0)->src == _db_1)
................    {
................      tree _cond_lhs_1 = gimple_cond_lhs (_ct_1);
................      tree _cond_rhs_1 = gimple_cond_rhs (_ct_1);
................      tree _p0 = build2 (gimple_cond_code (_ct_1), boolean_type_node, _cond_lhs_1, _cond_rhs_1);
................      bool _arg_0_is_true_1 = gimple_phi_arg_edge (_a1, 0)->flags & EDGE_TRUE_VALUE;
................      tree _p1 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 0 : 1);
................      tree _p2 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 1 : 0);
................      switch (TREE_CODE (_p0))
................        {

After this patch:
................  basic_block _pb_0_1 = EDGE_PRED (_b1, 0)->src;
................  basic_block _pb_1_1 = EDGE_PRED (_b1, 1)->src;
................  gcond *_ct_0_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1));
................  gcond *_ct_1_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_1_1));
................  gcond *_ct_a_1 = _ct_0_1 ? _ct_0_1 : _ct_1_1;
................  basic_block _db_1 = _ct_0_1 ? _pb_0_1 : _pb_1_1;
................  basic_block _other_db_1 = _ct_0_1 ? _pb_1_1 : _pb_0_1;
................  edge _e_00_1 = _pb_0_1->preds ? EDGE_PRED (_pb_0_1, 0) : NULL;
................  basic_block _pb_00_1 = _e_00_1 ? _e_00_1->src : NULL;
................  gcond *_ct_b_1 = _pb_00_1 ? safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_00_1)) : NULL;
................  if ((_ct_a_1 && EDGE_COUNT (_other_db_1->preds) == 1
................       && EDGE_COUNT (_other_db_1->succs) == 1
................       && EDGE_PRED (_other_db_1, 0)->src == _db_1)
................      ||
................      (_ct_b_1 && _pb_00_1 && EDGE_COUNT (_pb_0_1->succs) == 1
................       && EDGE_COUNT (_pb_0_1->preds) == 1
................       && EDGE_COUNT (_other_db_1->preds) == 1
................       && EDGE_COUNT (_other_db_1->succs) == 1
................       && EDGE_PRED (_other_db_1, 0)->src == _pb_00_1))
................    {
................      gcond *_ct_1 = _ct_a_1 ? _ct_a_1 : _ct_b_1;
................      tree _cond_lhs_1 = gimple_cond_lhs (_ct_1);
................      tree _cond_rhs_1 = gimple_cond_rhs (_ct_1);
................      tree _p0 = build2 (gimple_cond_code (_ct_1), boolean_type_node, _cond_lhs_1, _cond_rhs_1);
................      bool _arg_0_is_true_1 = gimple_phi_arg_edge (_a1, 0)->flags & EDGE_TRUE_VALUE;
................      tree _p1 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 0 : 1);
................      tree _p2 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 1 : 0);

Pan

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Wednesday, September 4, 2024 3:42 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@gmail.com
Subject: Re: [PATCH v1 1/2] Genmatch: Support new flow for phi on condition

On Wed, Sep 4, 2024 at 9:25 AM <pan2.li@intel.com> wrote:
>
> From: Pan Li <pan2.li@intel.com>
>
> The gen_phi_on_cond can only support below control flow for cond
> from day 1.  Aka:
>
> +------+
> | def  |
> | ...  |       +-----+
> | cond |------>| def |
> +------+       | ... |
>    |           +-----+
>    |              |
>    v              |
> +-----+           |
> | PHI |<----------+
> +-----+
>
> Unfortunately, there will be more scenarios of control flow on PHI.
> For example as below:
>
> T __attribute__((noinline))                            \
> sat_s_add_##T##_fmt_3 (T x, T y)                       \
> {                                                      \
>   T sum;                                               \
>   bool overflow = __builtin_add_overflow (x, y, &sum); \
>   return overflow ? x < 0 ? MIN : MAX : sum;           \
> }
>
> DEF_SAT_S_ADD_FMT_3(int8_t, uint8_t, INT8_MIN, INT8_MAX)
>
> With expanded RTL like below.
>    3   │
>    4   │ __attribute__((noinline))
>    5   │ int8_t sat_s_add_int8_t_fmt_3 (int8_t x, int8_t y)
>    6   │ {
>    7   │   signed char _1;
>    8   │   signed char _2;
>    9   │   int8_t _3;
>   10   │   __complex__ signed char _6;
>   11   │   _Bool _8;
>   12   │   signed char _9;
>   13   │   signed char _10;
>   14   │   signed char _11;
>   15   │
>   16   │ ;;   basic block 2, loop depth 0
>   17   │ ;;    pred:       ENTRY
>   18   │   _6 = .ADD_OVERFLOW (x_4(D), y_5(D));
>   19   │   _2 = IMAGPART_EXPR <_6>;
>   20   │   if (_2 != 0)
>   21   │     goto <bb 4>; [50.00%]
>   22   │   else
>   23   │     goto <bb 3>; [50.00%]
>   24   │ ;;    succ:       4
>   25   │ ;;                3
>   26   │
>   27   │ ;;   basic block 3, loop depth 0
>   28   │ ;;    pred:       2
>   29   │   _1 = REALPART_EXPR <_6>;
>   30   │   goto <bb 5>; [100.00%]
>   31   │ ;;    succ:       5
>   32   │
>   33   │ ;;   basic block 4, loop depth 0
>   34   │ ;;    pred:       2
>   35   │   _8 = x_4(D) < 0;
>   36   │   _9 = (signed char) _8;
>   37   │   _10 = -_9;
>   38   │   _11 = _10 ^ 127;
>   39   │ ;;    succ:       5
>   40   │
>   41   │ ;;   basic block 5, loop depth 0
>   42   │ ;;    pred:       3
>   43   │ ;;                4
>   44   │   # _3 = PHI <_1(3), _11(4)>
>   45   │   return _3;
>   46   │ ;;    succ:       EXIT
>   47   │
>   48   │ }
>
> The above code will have below control flow which is not supported by
> the gen_phi_on_cond.
>
> +------+
> | def  |
> | ...  |       +-----+
> | cond |------>| def |
> +------+       | ... |
>    |           +-----+
>    |              |
>    v              |
> +-----+           |
> | def |           |
> | ... |           |
> +-----+           |
>    |              |
>    |              |
>    v              |
> +-----+           |
> | PHI |<----------+
> +-----+
>
> This patch would like to add support above control flow for the
> gen_phi_on_cond.
>
> The below testsuites are passed for this patch:
> * The rv64gcv fully regression test.
> * The x86 bootstrap test.
> * The x86 fully regression test.

I'm lazy - can you please quote genmatch generated code for the condition for
one case?

Thanks,
Richard.

> gcc/ChangeLog:
>
>         * genmatch.cc (dt_operand::gen_phi_on_cond): Add support for
>         a new control flow when gen phi on condition.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
>  gcc/genmatch.cc | 85 +++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 76 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/genmatch.cc b/gcc/genmatch.cc
> index a56bd90cb2c..f538df1be62 100644
> --- a/gcc/genmatch.cc
> +++ b/gcc/genmatch.cc
> @@ -3529,28 +3529,95 @@ dt_operand::gen_phi_on_cond (FILE *f, int indent, int depth)
>      "basic_block _pb_0_%d = EDGE_PRED (_b%d, 0)->src;\n", depth, depth);
>    fprintf_indent (f, indent,
>      "basic_block _pb_1_%d = EDGE_PRED (_b%d, 1)->src;\n", depth, depth);
> +
>    fprintf_indent (f, indent,
> -    "basic_block _db_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d)) ? "
> -    "_pb_0_%d : _pb_1_%d;\n", depth, depth, depth, depth);
> +    "gcond *_ct_0_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d));\n",
> +    depth, depth);
>    fprintf_indent (f, indent,
> -    "basic_block _other_db_%d = safe_dyn_cast <gcond *> "
> -    "(*gsi_last_bb (_pb_0_%d)) ? _pb_1_%d : _pb_0_%d;\n",
> +    "gcond *_ct_1_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_1_%d));\n",
> +    depth, depth);
> +  fprintf_indent (f, indent,
> +    "gcond *_ct_a_%d = _ct_0_%d ? _ct_0_%d : _ct_1_%d;\n",
> +    depth, depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "basic_block _db_%d = _ct_0_%d ? _pb_0_%d : _pb_1_%d;\n",
> +    depth, depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "basic_block _other_db_%d = _ct_0_%d ? _pb_1_%d : _pb_0_%d;\n",
>      depth, depth, depth, depth);
>
>    fprintf_indent (f, indent,
> -    "gcond *_ct_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_db_%d));\n",
> -    depth, depth);
> -  fprintf_indent (f, indent, "if (_ct_%d"
> +    "edge _e_00_%d = _pb_0_%d->preds ? EDGE_PRED (_pb_0_%d, 0) : NULL;\n",
> +    depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "basic_block _pb_00_%d = _e_00_%d ? _e_00_%d->src : NULL;\n",
> +    depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "gcond *_ct_b_%d = _pb_00_%d ? "
> +    "safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_00_%d)) : NULL;\n",
> +    depth, depth, depth);
> +
> +  /* Case 1 flow for PHI.
> +   * +------+
> +   * | def  |
> +   * | ...  |       +-----+
> +   * | cond |------>| def |
> +   * +------+       | ... |
> +   *    |           +-----+
> +   *    |              |
> +   *    v              |
> +   * +-----+           |
> +   * | PHI |<----------+
> +   * +-----+
> +   */
> +  fprintf_indent (f, indent, "if ((_ct_a_%d"
>      " && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth, depth);
>    fprintf_indent (f, indent,
> -    "  && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> +    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> +  fprintf_indent (f, indent,
> +    "     && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
> +
> +  fprintf_indent (f, indent, "    ||\n");
> +
> +  /* Case 2 flow for PHI.
> +   * +------+
> +   * | def  |
> +   * | ...  |       +-----+
> +   * | cond |------>| def |
> +   * +------+       | ... |
> +   *    |           +-----+
> +   *    |              |
> +   *    v              |
> +   * +-----+           |
> +   * | def |           |
> +   * | ... |           |
> +   * +-----+           |
> +   *    |              |
> +   *    |              |
> +   *    v              |
> +   * +-----+           |
> +   * | PHI |<----------+
> +   * +-----+
> +   */
> +  fprintf_indent (f, indent,
> +    "    (_ct_b_%d && _pb_00_%d && EDGE_COUNT (_pb_0_%d->succs) == 1\n",
> +    depth, depth, depth);
> +  fprintf_indent (f, indent,
> +    "     && EDGE_COUNT (_pb_0_%d->preds) == 1\n", depth);
>    fprintf_indent (f, indent,
> -    "  && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
> +    "     && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth);
> +  fprintf_indent (f, indent,
> +    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> +  fprintf_indent (f, indent,
> +    "     && EDGE_PRED (_other_db_%d, 0)->src == _pb_00_%d))\n", depth, depth);
>
>    indent += 2;
>    fprintf_indent (f, indent, "{\n");
>    indent += 2;
>
> +  fprintf_indent (f, indent,
> +    "gcond *_ct_%d = _ct_a_%d ? _ct_a_%d : _ct_b_%d;\n",
> +    depth, depth, depth, depth);
>    fprintf_indent (f, indent,
>      "tree _cond_lhs_%d = gimple_cond_lhs (_ct_%d);\n", depth, depth);
>    fprintf_indent (f, indent,
> --
> 2.43.0
>
Richard Biener Sept. 4, 2024, 10:55 a.m. UTC | #3
On Wed, Sep 4, 2024 at 9:48 AM Li, Pan2 <pan2.li@intel.com> wrote:
>
> > I'm lazy - can you please quote genmatch generated code for the condition for
> > one case?
>
> Sure thing, list the before and after covers all the changes to generated code as blow.
>
> Before this patch:
> ........      basic_block _b1 = gimple_bb (_a1);
> ........      if (gimple_phi_num_args (_a1) == 2)
> ................{
> ................  basic_block _pb_0_1 = EDGE_PRED (_b1, 0)->src;
> ................  basic_block _pb_1_1 = EDGE_PRED (_b1, 1)->src;
> ................  basic_block _db_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1)) ? _pb_0_1 : _pb_1_1;
> ................  basic_block _other_db_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1)) ? _pb_1_1 : _pb_0_1;
> ................  gcond *_ct_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_db_1));
> ................  if (_ct_1 && EDGE_COUNT (_other_db_1->preds) == 1
> ................    && EDGE_COUNT (_other_db_1->succs) == 1
> ................    && EDGE_PRED (_other_db_1, 0)->src == _db_1)
> ................    {
> ................      tree _cond_lhs_1 = gimple_cond_lhs (_ct_1);
> ................      tree _cond_rhs_1 = gimple_cond_rhs (_ct_1);
> ................      tree _p0 = build2 (gimple_cond_code (_ct_1), boolean_type_node, _cond_lhs_1, _cond_rhs_1);
> ................      bool _arg_0_is_true_1 = gimple_phi_arg_edge (_a1, 0)->flags & EDGE_TRUE_VALUE;
> ................      tree _p1 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 0 : 1);
> ................      tree _p2 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 1 : 0);
> ................      switch (TREE_CODE (_p0))
> ................        {
>
> After this patch:
> ................  basic_block _pb_0_1 = EDGE_PRED (_b1, 0)->src;
> ................  basic_block _pb_1_1 = EDGE_PRED (_b1, 1)->src;
> ................  gcond *_ct_0_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1));
> ................  gcond *_ct_1_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_1_1));
> ................  gcond *_ct_a_1 = _ct_0_1 ? _ct_0_1 : _ct_1_1;
> ................  basic_block _db_1 = _ct_0_1 ? _pb_0_1 : _pb_1_1;
> ................  basic_block _other_db_1 = _ct_0_1 ? _pb_1_1 : _pb_0_1;
> ................  edge _e_00_1 = _pb_0_1->preds ? EDGE_PRED (_pb_0_1, 0) : NULL;
> ................  basic_block _pb_00_1 = _e_00_1 ? _e_00_1->src : NULL;
> ................  gcond *_ct_b_1 = _pb_00_1 ? safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_00_1)) : NULL;
> ................  if ((_ct_a_1 && EDGE_COUNT (_other_db_1->preds) == 1
> ................       && EDGE_COUNT (_other_db_1->succs) == 1
> ................       && EDGE_PRED (_other_db_1, 0)->src == _db_1)
> ................      ||
> ................      (_ct_b_1 && _pb_00_1 && EDGE_COUNT (_pb_0_1->succs) == 1
> ................       && EDGE_COUNT (_pb_0_1->preds) == 1
> ................       && EDGE_COUNT (_other_db_1->preds) == 1
> ................       && EDGE_COUNT (_other_db_1->succs) == 1
> ................       && EDGE_PRED (_other_db_1, 0)->src == _pb_00_1))
> ................    {
> ................      gcond *_ct_1 = _ct_a_1 ? _ct_a_1 : _ct_b_1;
> ................      tree _cond_lhs_1 = gimple_cond_lhs (_ct_1);
> ................      tree _cond_rhs_1 = gimple_cond_rhs (_ct_1);
> ................      tree _p0 = build2 (gimple_cond_code (_ct_1), boolean_type_node, _cond_lhs_1, _cond_rhs_1);
> ................      bool _arg_0_is_true_1 = gimple_phi_arg_edge (_a1, 0)->flags & EDGE_TRUE_VALUE;
> ................      tree _p1 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 0 : 1);
> ................      tree _p2 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 1 : 0);

I think it might be better to refactor this to detect the three CFGs like

     if (EDGE_COUNT (_pb_0_1->preds) == 1
         && EDGE_PRED (_pb_0_1, 0)->src == pb_1_1)
       {
.. check rest of constraints ..
       }
    else if (... same for _pb_1_1 being the forwarder ...)
 ...
    else if (EDGE_COUNT (_pb_0_1->preds) == 1
               && EDGE_COUNT (_pb_1_1->preds) == 1
               && EDGE_PRED (_pb_0_1, 0)->src == EDGE_PRED (_pb_1_1, 0)->src)
...

I also think we may want to split out this CFG matching code out into
a helper function
in gimple-match-head.cc instead of repeating it fully for each pattern?

Thanks,
Richard.

> Pan
>
> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Wednesday, September 4, 2024 3:42 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@gmail.com
> Subject: Re: [PATCH v1 1/2] Genmatch: Support new flow for phi on condition
>
> On Wed, Sep 4, 2024 at 9:25 AM <pan2.li@intel.com> wrote:
> >
> > From: Pan Li <pan2.li@intel.com>
> >
> > The gen_phi_on_cond can only support below control flow for cond
> > from day 1.  Aka:
> >
> > +------+
> > | def  |
> > | ...  |       +-----+
> > | cond |------>| def |
> > +------+       | ... |
> >    |           +-----+
> >    |              |
> >    v              |
> > +-----+           |
> > | PHI |<----------+
> > +-----+
> >
> > Unfortunately, there will be more scenarios of control flow on PHI.
> > For example as below:
> >
> > T __attribute__((noinline))                            \
> > sat_s_add_##T##_fmt_3 (T x, T y)                       \
> > {                                                      \
> >   T sum;                                               \
> >   bool overflow = __builtin_add_overflow (x, y, &sum); \
> >   return overflow ? x < 0 ? MIN : MAX : sum;           \
> > }
> >
> > DEF_SAT_S_ADD_FMT_3(int8_t, uint8_t, INT8_MIN, INT8_MAX)
> >
> > With expanded RTL like below.
> >    3   │
> >    4   │ __attribute__((noinline))
> >    5   │ int8_t sat_s_add_int8_t_fmt_3 (int8_t x, int8_t y)
> >    6   │ {
> >    7   │   signed char _1;
> >    8   │   signed char _2;
> >    9   │   int8_t _3;
> >   10   │   __complex__ signed char _6;
> >   11   │   _Bool _8;
> >   12   │   signed char _9;
> >   13   │   signed char _10;
> >   14   │   signed char _11;
> >   15   │
> >   16   │ ;;   basic block 2, loop depth 0
> >   17   │ ;;    pred:       ENTRY
> >   18   │   _6 = .ADD_OVERFLOW (x_4(D), y_5(D));
> >   19   │   _2 = IMAGPART_EXPR <_6>;
> >   20   │   if (_2 != 0)
> >   21   │     goto <bb 4>; [50.00%]
> >   22   │   else
> >   23   │     goto <bb 3>; [50.00%]
> >   24   │ ;;    succ:       4
> >   25   │ ;;                3
> >   26   │
> >   27   │ ;;   basic block 3, loop depth 0
> >   28   │ ;;    pred:       2
> >   29   │   _1 = REALPART_EXPR <_6>;
> >   30   │   goto <bb 5>; [100.00%]
> >   31   │ ;;    succ:       5
> >   32   │
> >   33   │ ;;   basic block 4, loop depth 0
> >   34   │ ;;    pred:       2
> >   35   │   _8 = x_4(D) < 0;
> >   36   │   _9 = (signed char) _8;
> >   37   │   _10 = -_9;
> >   38   │   _11 = _10 ^ 127;
> >   39   │ ;;    succ:       5
> >   40   │
> >   41   │ ;;   basic block 5, loop depth 0
> >   42   │ ;;    pred:       3
> >   43   │ ;;                4
> >   44   │   # _3 = PHI <_1(3), _11(4)>
> >   45   │   return _3;
> >   46   │ ;;    succ:       EXIT
> >   47   │
> >   48   │ }
> >
> > The above code will have below control flow which is not supported by
> > the gen_phi_on_cond.
> >
> > +------+
> > | def  |
> > | ...  |       +-----+
> > | cond |------>| def |
> > +------+       | ... |
> >    |           +-----+
> >    |              |
> >    v              |
> > +-----+           |
> > | def |           |
> > | ... |           |
> > +-----+           |
> >    |              |
> >    |              |
> >    v              |
> > +-----+           |
> > | PHI |<----------+
> > +-----+
> >
> > This patch would like to add support above control flow for the
> > gen_phi_on_cond.
> >
> > The below testsuites are passed for this patch:
> > * The rv64gcv fully regression test.
> > * The x86 bootstrap test.
> > * The x86 fully regression test.
>
> I'm lazy - can you please quote genmatch generated code for the condition for
> one case?
>
> Thanks,
> Richard.
>
> > gcc/ChangeLog:
> >
> >         * genmatch.cc (dt_operand::gen_phi_on_cond): Add support for
> >         a new control flow when gen phi on condition.
> >
> > Signed-off-by: Pan Li <pan2.li@intel.com>
> > ---
> >  gcc/genmatch.cc | 85 +++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 76 insertions(+), 9 deletions(-)
> >
> > diff --git a/gcc/genmatch.cc b/gcc/genmatch.cc
> > index a56bd90cb2c..f538df1be62 100644
> > --- a/gcc/genmatch.cc
> > +++ b/gcc/genmatch.cc
> > @@ -3529,28 +3529,95 @@ dt_operand::gen_phi_on_cond (FILE *f, int indent, int depth)
> >      "basic_block _pb_0_%d = EDGE_PRED (_b%d, 0)->src;\n", depth, depth);
> >    fprintf_indent (f, indent,
> >      "basic_block _pb_1_%d = EDGE_PRED (_b%d, 1)->src;\n", depth, depth);
> > +
> >    fprintf_indent (f, indent,
> > -    "basic_block _db_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d)) ? "
> > -    "_pb_0_%d : _pb_1_%d;\n", depth, depth, depth, depth);
> > +    "gcond *_ct_0_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d));\n",
> > +    depth, depth);
> >    fprintf_indent (f, indent,
> > -    "basic_block _other_db_%d = safe_dyn_cast <gcond *> "
> > -    "(*gsi_last_bb (_pb_0_%d)) ? _pb_1_%d : _pb_0_%d;\n",
> > +    "gcond *_ct_1_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_1_%d));\n",
> > +    depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "gcond *_ct_a_%d = _ct_0_%d ? _ct_0_%d : _ct_1_%d;\n",
> > +    depth, depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "basic_block _db_%d = _ct_0_%d ? _pb_0_%d : _pb_1_%d;\n",
> > +    depth, depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "basic_block _other_db_%d = _ct_0_%d ? _pb_1_%d : _pb_0_%d;\n",
> >      depth, depth, depth, depth);
> >
> >    fprintf_indent (f, indent,
> > -    "gcond *_ct_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_db_%d));\n",
> > -    depth, depth);
> > -  fprintf_indent (f, indent, "if (_ct_%d"
> > +    "edge _e_00_%d = _pb_0_%d->preds ? EDGE_PRED (_pb_0_%d, 0) : NULL;\n",
> > +    depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "basic_block _pb_00_%d = _e_00_%d ? _e_00_%d->src : NULL;\n",
> > +    depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "gcond *_ct_b_%d = _pb_00_%d ? "
> > +    "safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_00_%d)) : NULL;\n",
> > +    depth, depth, depth);
> > +
> > +  /* Case 1 flow for PHI.
> > +   * +------+
> > +   * | def  |
> > +   * | ...  |       +-----+
> > +   * | cond |------>| def |
> > +   * +------+       | ... |
> > +   *    |           +-----+
> > +   *    |              |
> > +   *    v              |
> > +   * +-----+           |
> > +   * | PHI |<----------+
> > +   * +-----+
> > +   */
> > +  fprintf_indent (f, indent, "if ((_ct_a_%d"
> >      " && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth, depth);
> >    fprintf_indent (f, indent,
> > -    "  && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> > +    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> > +  fprintf_indent (f, indent,
> > +    "     && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
> > +
> > +  fprintf_indent (f, indent, "    ||\n");
> > +
> > +  /* Case 2 flow for PHI.
> > +   * +------+
> > +   * | def  |
> > +   * | ...  |       +-----+
> > +   * | cond |------>| def |
> > +   * +------+       | ... |
> > +   *    |           +-----+
> > +   *    |              |
> > +   *    v              |
> > +   * +-----+           |
> > +   * | def |           |
> > +   * | ... |           |
> > +   * +-----+           |
> > +   *    |              |
> > +   *    |              |
> > +   *    v              |
> > +   * +-----+           |
> > +   * | PHI |<----------+
> > +   * +-----+
> > +   */
> > +  fprintf_indent (f, indent,
> > +    "    (_ct_b_%d && _pb_00_%d && EDGE_COUNT (_pb_0_%d->succs) == 1\n",
> > +    depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "     && EDGE_COUNT (_pb_0_%d->preds) == 1\n", depth);
> >    fprintf_indent (f, indent,
> > -    "  && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
> > +    "     && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth);
> > +  fprintf_indent (f, indent,
> > +    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> > +  fprintf_indent (f, indent,
> > +    "     && EDGE_PRED (_other_db_%d, 0)->src == _pb_00_%d))\n", depth, depth);
> >
> >    indent += 2;
> >    fprintf_indent (f, indent, "{\n");
> >    indent += 2;
> >
> > +  fprintf_indent (f, indent,
> > +    "gcond *_ct_%d = _ct_a_%d ? _ct_a_%d : _ct_b_%d;\n",
> > +    depth, depth, depth, depth);
> >    fprintf_indent (f, indent,
> >      "tree _cond_lhs_%d = gimple_cond_lhs (_ct_%d);\n", depth, depth);
> >    fprintf_indent (f, indent,
> > --
> > 2.43.0
> >
Li, Pan2 Sept. 4, 2024, 11:07 p.m. UTC | #4
Thanks Richard for comments.

> I also think we may want to split out this CFG matching code out into
> a helper function
> in gimple-match-head.cc instead of repeating it fully for each pattern?

That makes sense to me, let me have a try in v2.

Pan

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Wednesday, September 4, 2024 6:56 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@gmail.com
Subject: Re: [PATCH v1 1/2] Genmatch: Support new flow for phi on condition

On Wed, Sep 4, 2024 at 9:48 AM Li, Pan2 <pan2.li@intel.com> wrote:
>
> > I'm lazy - can you please quote genmatch generated code for the condition for
> > one case?
>
> Sure thing, list the before and after covers all the changes to generated code as blow.
>
> Before this patch:
> ........      basic_block _b1 = gimple_bb (_a1);
> ........      if (gimple_phi_num_args (_a1) == 2)
> ................{
> ................  basic_block _pb_0_1 = EDGE_PRED (_b1, 0)->src;
> ................  basic_block _pb_1_1 = EDGE_PRED (_b1, 1)->src;
> ................  basic_block _db_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1)) ? _pb_0_1 : _pb_1_1;
> ................  basic_block _other_db_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1)) ? _pb_1_1 : _pb_0_1;
> ................  gcond *_ct_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_db_1));
> ................  if (_ct_1 && EDGE_COUNT (_other_db_1->preds) == 1
> ................    && EDGE_COUNT (_other_db_1->succs) == 1
> ................    && EDGE_PRED (_other_db_1, 0)->src == _db_1)
> ................    {
> ................      tree _cond_lhs_1 = gimple_cond_lhs (_ct_1);
> ................      tree _cond_rhs_1 = gimple_cond_rhs (_ct_1);
> ................      tree _p0 = build2 (gimple_cond_code (_ct_1), boolean_type_node, _cond_lhs_1, _cond_rhs_1);
> ................      bool _arg_0_is_true_1 = gimple_phi_arg_edge (_a1, 0)->flags & EDGE_TRUE_VALUE;
> ................      tree _p1 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 0 : 1);
> ................      tree _p2 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 1 : 0);
> ................      switch (TREE_CODE (_p0))
> ................        {
>
> After this patch:
> ................  basic_block _pb_0_1 = EDGE_PRED (_b1, 0)->src;
> ................  basic_block _pb_1_1 = EDGE_PRED (_b1, 1)->src;
> ................  gcond *_ct_0_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_1));
> ................  gcond *_ct_1_1 = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_1_1));
> ................  gcond *_ct_a_1 = _ct_0_1 ? _ct_0_1 : _ct_1_1;
> ................  basic_block _db_1 = _ct_0_1 ? _pb_0_1 : _pb_1_1;
> ................  basic_block _other_db_1 = _ct_0_1 ? _pb_1_1 : _pb_0_1;
> ................  edge _e_00_1 = _pb_0_1->preds ? EDGE_PRED (_pb_0_1, 0) : NULL;
> ................  basic_block _pb_00_1 = _e_00_1 ? _e_00_1->src : NULL;
> ................  gcond *_ct_b_1 = _pb_00_1 ? safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_00_1)) : NULL;
> ................  if ((_ct_a_1 && EDGE_COUNT (_other_db_1->preds) == 1
> ................       && EDGE_COUNT (_other_db_1->succs) == 1
> ................       && EDGE_PRED (_other_db_1, 0)->src == _db_1)
> ................      ||
> ................      (_ct_b_1 && _pb_00_1 && EDGE_COUNT (_pb_0_1->succs) == 1
> ................       && EDGE_COUNT (_pb_0_1->preds) == 1
> ................       && EDGE_COUNT (_other_db_1->preds) == 1
> ................       && EDGE_COUNT (_other_db_1->succs) == 1
> ................       && EDGE_PRED (_other_db_1, 0)->src == _pb_00_1))
> ................    {
> ................      gcond *_ct_1 = _ct_a_1 ? _ct_a_1 : _ct_b_1;
> ................      tree _cond_lhs_1 = gimple_cond_lhs (_ct_1);
> ................      tree _cond_rhs_1 = gimple_cond_rhs (_ct_1);
> ................      tree _p0 = build2 (gimple_cond_code (_ct_1), boolean_type_node, _cond_lhs_1, _cond_rhs_1);
> ................      bool _arg_0_is_true_1 = gimple_phi_arg_edge (_a1, 0)->flags & EDGE_TRUE_VALUE;
> ................      tree _p1 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 0 : 1);
> ................      tree _p2 = gimple_phi_arg_def (_a1, _arg_0_is_true_1 ? 1 : 0);

I think it might be better to refactor this to detect the three CFGs like

     if (EDGE_COUNT (_pb_0_1->preds) == 1
         && EDGE_PRED (_pb_0_1, 0)->src == pb_1_1)
       {
.. check rest of constraints ..
       }
    else if (... same for _pb_1_1 being the forwarder ...)
 ...
    else if (EDGE_COUNT (_pb_0_1->preds) == 1
               && EDGE_COUNT (_pb_1_1->preds) == 1
               && EDGE_PRED (_pb_0_1, 0)->src == EDGE_PRED (_pb_1_1, 0)->src)
...

I also think we may want to split out this CFG matching code out into
a helper function
in gimple-match-head.cc instead of repeating it fully for each pattern?

Thanks,
Richard.

> Pan
>
> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Wednesday, September 4, 2024 3:42 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@gmail.com
> Subject: Re: [PATCH v1 1/2] Genmatch: Support new flow for phi on condition
>
> On Wed, Sep 4, 2024 at 9:25 AM <pan2.li@intel.com> wrote:
> >
> > From: Pan Li <pan2.li@intel.com>
> >
> > The gen_phi_on_cond can only support below control flow for cond
> > from day 1.  Aka:
> >
> > +------+
> > | def  |
> > | ...  |       +-----+
> > | cond |------>| def |
> > +------+       | ... |
> >    |           +-----+
> >    |              |
> >    v              |
> > +-----+           |
> > | PHI |<----------+
> > +-----+
> >
> > Unfortunately, there will be more scenarios of control flow on PHI.
> > For example as below:
> >
> > T __attribute__((noinline))                            \
> > sat_s_add_##T##_fmt_3 (T x, T y)                       \
> > {                                                      \
> >   T sum;                                               \
> >   bool overflow = __builtin_add_overflow (x, y, &sum); \
> >   return overflow ? x < 0 ? MIN : MAX : sum;           \
> > }
> >
> > DEF_SAT_S_ADD_FMT_3(int8_t, uint8_t, INT8_MIN, INT8_MAX)
> >
> > With expanded RTL like below.
> >    3   │
> >    4   │ __attribute__((noinline))
> >    5   │ int8_t sat_s_add_int8_t_fmt_3 (int8_t x, int8_t y)
> >    6   │ {
> >    7   │   signed char _1;
> >    8   │   signed char _2;
> >    9   │   int8_t _3;
> >   10   │   __complex__ signed char _6;
> >   11   │   _Bool _8;
> >   12   │   signed char _9;
> >   13   │   signed char _10;
> >   14   │   signed char _11;
> >   15   │
> >   16   │ ;;   basic block 2, loop depth 0
> >   17   │ ;;    pred:       ENTRY
> >   18   │   _6 = .ADD_OVERFLOW (x_4(D), y_5(D));
> >   19   │   _2 = IMAGPART_EXPR <_6>;
> >   20   │   if (_2 != 0)
> >   21   │     goto <bb 4>; [50.00%]
> >   22   │   else
> >   23   │     goto <bb 3>; [50.00%]
> >   24   │ ;;    succ:       4
> >   25   │ ;;                3
> >   26   │
> >   27   │ ;;   basic block 3, loop depth 0
> >   28   │ ;;    pred:       2
> >   29   │   _1 = REALPART_EXPR <_6>;
> >   30   │   goto <bb 5>; [100.00%]
> >   31   │ ;;    succ:       5
> >   32   │
> >   33   │ ;;   basic block 4, loop depth 0
> >   34   │ ;;    pred:       2
> >   35   │   _8 = x_4(D) < 0;
> >   36   │   _9 = (signed char) _8;
> >   37   │   _10 = -_9;
> >   38   │   _11 = _10 ^ 127;
> >   39   │ ;;    succ:       5
> >   40   │
> >   41   │ ;;   basic block 5, loop depth 0
> >   42   │ ;;    pred:       3
> >   43   │ ;;                4
> >   44   │   # _3 = PHI <_1(3), _11(4)>
> >   45   │   return _3;
> >   46   │ ;;    succ:       EXIT
> >   47   │
> >   48   │ }
> >
> > The above code will have below control flow which is not supported by
> > the gen_phi_on_cond.
> >
> > +------+
> > | def  |
> > | ...  |       +-----+
> > | cond |------>| def |
> > +------+       | ... |
> >    |           +-----+
> >    |              |
> >    v              |
> > +-----+           |
> > | def |           |
> > | ... |           |
> > +-----+           |
> >    |              |
> >    |              |
> >    v              |
> > +-----+           |
> > | PHI |<----------+
> > +-----+
> >
> > This patch would like to add support above control flow for the
> > gen_phi_on_cond.
> >
> > The below testsuites are passed for this patch:
> > * The rv64gcv fully regression test.
> > * The x86 bootstrap test.
> > * The x86 fully regression test.
>
> I'm lazy - can you please quote genmatch generated code for the condition for
> one case?
>
> Thanks,
> Richard.
>
> > gcc/ChangeLog:
> >
> >         * genmatch.cc (dt_operand::gen_phi_on_cond): Add support for
> >         a new control flow when gen phi on condition.
> >
> > Signed-off-by: Pan Li <pan2.li@intel.com>
> > ---
> >  gcc/genmatch.cc | 85 +++++++++++++++++++++++++++++++++++++++++++------
> >  1 file changed, 76 insertions(+), 9 deletions(-)
> >
> > diff --git a/gcc/genmatch.cc b/gcc/genmatch.cc
> > index a56bd90cb2c..f538df1be62 100644
> > --- a/gcc/genmatch.cc
> > +++ b/gcc/genmatch.cc
> > @@ -3529,28 +3529,95 @@ dt_operand::gen_phi_on_cond (FILE *f, int indent, int depth)
> >      "basic_block _pb_0_%d = EDGE_PRED (_b%d, 0)->src;\n", depth, depth);
> >    fprintf_indent (f, indent,
> >      "basic_block _pb_1_%d = EDGE_PRED (_b%d, 1)->src;\n", depth, depth);
> > +
> >    fprintf_indent (f, indent,
> > -    "basic_block _db_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d)) ? "
> > -    "_pb_0_%d : _pb_1_%d;\n", depth, depth, depth, depth);
> > +    "gcond *_ct_0_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d));\n",
> > +    depth, depth);
> >    fprintf_indent (f, indent,
> > -    "basic_block _other_db_%d = safe_dyn_cast <gcond *> "
> > -    "(*gsi_last_bb (_pb_0_%d)) ? _pb_1_%d : _pb_0_%d;\n",
> > +    "gcond *_ct_1_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_1_%d));\n",
> > +    depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "gcond *_ct_a_%d = _ct_0_%d ? _ct_0_%d : _ct_1_%d;\n",
> > +    depth, depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "basic_block _db_%d = _ct_0_%d ? _pb_0_%d : _pb_1_%d;\n",
> > +    depth, depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "basic_block _other_db_%d = _ct_0_%d ? _pb_1_%d : _pb_0_%d;\n",
> >      depth, depth, depth, depth);
> >
> >    fprintf_indent (f, indent,
> > -    "gcond *_ct_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_db_%d));\n",
> > -    depth, depth);
> > -  fprintf_indent (f, indent, "if (_ct_%d"
> > +    "edge _e_00_%d = _pb_0_%d->preds ? EDGE_PRED (_pb_0_%d, 0) : NULL;\n",
> > +    depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "basic_block _pb_00_%d = _e_00_%d ? _e_00_%d->src : NULL;\n",
> > +    depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "gcond *_ct_b_%d = _pb_00_%d ? "
> > +    "safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_00_%d)) : NULL;\n",
> > +    depth, depth, depth);
> > +
> > +  /* Case 1 flow for PHI.
> > +   * +------+
> > +   * | def  |
> > +   * | ...  |       +-----+
> > +   * | cond |------>| def |
> > +   * +------+       | ... |
> > +   *    |           +-----+
> > +   *    |              |
> > +   *    v              |
> > +   * +-----+           |
> > +   * | PHI |<----------+
> > +   * +-----+
> > +   */
> > +  fprintf_indent (f, indent, "if ((_ct_a_%d"
> >      " && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth, depth);
> >    fprintf_indent (f, indent,
> > -    "  && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> > +    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> > +  fprintf_indent (f, indent,
> > +    "     && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
> > +
> > +  fprintf_indent (f, indent, "    ||\n");
> > +
> > +  /* Case 2 flow for PHI.
> > +   * +------+
> > +   * | def  |
> > +   * | ...  |       +-----+
> > +   * | cond |------>| def |
> > +   * +------+       | ... |
> > +   *    |           +-----+
> > +   *    |              |
> > +   *    v              |
> > +   * +-----+           |
> > +   * | def |           |
> > +   * | ... |           |
> > +   * +-----+           |
> > +   *    |              |
> > +   *    |              |
> > +   *    v              |
> > +   * +-----+           |
> > +   * | PHI |<----------+
> > +   * +-----+
> > +   */
> > +  fprintf_indent (f, indent,
> > +    "    (_ct_b_%d && _pb_00_%d && EDGE_COUNT (_pb_0_%d->succs) == 1\n",
> > +    depth, depth, depth);
> > +  fprintf_indent (f, indent,
> > +    "     && EDGE_COUNT (_pb_0_%d->preds) == 1\n", depth);
> >    fprintf_indent (f, indent,
> > -    "  && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
> > +    "     && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth);
> > +  fprintf_indent (f, indent,
> > +    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
> > +  fprintf_indent (f, indent,
> > +    "     && EDGE_PRED (_other_db_%d, 0)->src == _pb_00_%d))\n", depth, depth);
> >
> >    indent += 2;
> >    fprintf_indent (f, indent, "{\n");
> >    indent += 2;
> >
> > +  fprintf_indent (f, indent,
> > +    "gcond *_ct_%d = _ct_a_%d ? _ct_a_%d : _ct_b_%d;\n",
> > +    depth, depth, depth, depth);
> >    fprintf_indent (f, indent,
> >      "tree _cond_lhs_%d = gimple_cond_lhs (_ct_%d);\n", depth, depth);
> >    fprintf_indent (f, indent,
> > --
> > 2.43.0
> >
diff mbox series

Patch

diff --git a/gcc/genmatch.cc b/gcc/genmatch.cc
index a56bd90cb2c..f538df1be62 100644
--- a/gcc/genmatch.cc
+++ b/gcc/genmatch.cc
@@ -3529,28 +3529,95 @@  dt_operand::gen_phi_on_cond (FILE *f, int indent, int depth)
     "basic_block _pb_0_%d = EDGE_PRED (_b%d, 0)->src;\n", depth, depth);
   fprintf_indent (f, indent,
     "basic_block _pb_1_%d = EDGE_PRED (_b%d, 1)->src;\n", depth, depth);
+
   fprintf_indent (f, indent,
-    "basic_block _db_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d)) ? "
-    "_pb_0_%d : _pb_1_%d;\n", depth, depth, depth, depth);
+    "gcond *_ct_0_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_0_%d));\n",
+    depth, depth);
   fprintf_indent (f, indent,
-    "basic_block _other_db_%d = safe_dyn_cast <gcond *> "
-    "(*gsi_last_bb (_pb_0_%d)) ? _pb_1_%d : _pb_0_%d;\n",
+    "gcond *_ct_1_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_1_%d));\n",
+    depth, depth);
+  fprintf_indent (f, indent,
+    "gcond *_ct_a_%d = _ct_0_%d ? _ct_0_%d : _ct_1_%d;\n",
+    depth, depth, depth, depth);
+  fprintf_indent (f, indent,
+    "basic_block _db_%d = _ct_0_%d ? _pb_0_%d : _pb_1_%d;\n",
+    depth, depth, depth, depth);
+  fprintf_indent (f, indent,
+    "basic_block _other_db_%d = _ct_0_%d ? _pb_1_%d : _pb_0_%d;\n",
     depth, depth, depth, depth);
 
   fprintf_indent (f, indent,
-    "gcond *_ct_%d = safe_dyn_cast <gcond *> (*gsi_last_bb (_db_%d));\n",
-    depth, depth);
-  fprintf_indent (f, indent, "if (_ct_%d"
+    "edge _e_00_%d = _pb_0_%d->preds ? EDGE_PRED (_pb_0_%d, 0) : NULL;\n",
+    depth, depth, depth);
+  fprintf_indent (f, indent,
+    "basic_block _pb_00_%d = _e_00_%d ? _e_00_%d->src : NULL;\n",
+    depth, depth, depth);
+  fprintf_indent (f, indent,
+    "gcond *_ct_b_%d = _pb_00_%d ? "
+    "safe_dyn_cast <gcond *> (*gsi_last_bb (_pb_00_%d)) : NULL;\n",
+    depth, depth, depth);
+
+  /* Case 1 flow for PHI.
+   * +------+
+   * | def  |
+   * | ...  |       +-----+
+   * | cond |------>| def |
+   * +------+       | ... |
+   *    |           +-----+
+   *    |              |
+   *    v              |
+   * +-----+           |
+   * | PHI |<----------+
+   * +-----+
+   */
+  fprintf_indent (f, indent, "if ((_ct_a_%d"
     " && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth, depth);
   fprintf_indent (f, indent,
-    "  && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
+    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
+  fprintf_indent (f, indent,
+    "     && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
+
+  fprintf_indent (f, indent, "    ||\n");
+
+  /* Case 2 flow for PHI.
+   * +------+
+   * | def  |
+   * | ...  |       +-----+
+   * | cond |------>| def |
+   * +------+       | ... |
+   *    |           +-----+
+   *    |              |
+   *    v              |
+   * +-----+           |
+   * | def |           |
+   * | ... |           |
+   * +-----+           |
+   *    |              |
+   *    |              |
+   *    v              |
+   * +-----+           |
+   * | PHI |<----------+
+   * +-----+
+   */
+  fprintf_indent (f, indent,
+    "    (_ct_b_%d && _pb_00_%d && EDGE_COUNT (_pb_0_%d->succs) == 1\n",
+    depth, depth, depth);
+  fprintf_indent (f, indent,
+    "     && EDGE_COUNT (_pb_0_%d->preds) == 1\n", depth);
   fprintf_indent (f, indent,
-    "  && EDGE_PRED (_other_db_%d, 0)->src == _db_%d)\n", depth, depth);
+    "     && EDGE_COUNT (_other_db_%d->preds) == 1\n", depth);
+  fprintf_indent (f, indent,
+    "     && EDGE_COUNT (_other_db_%d->succs) == 1\n", depth);
+  fprintf_indent (f, indent,
+    "     && EDGE_PRED (_other_db_%d, 0)->src == _pb_00_%d))\n", depth, depth);
 
   indent += 2;
   fprintf_indent (f, indent, "{\n");
   indent += 2;
 
+  fprintf_indent (f, indent,
+    "gcond *_ct_%d = _ct_a_%d ? _ct_a_%d : _ct_b_%d;\n",
+    depth, depth, depth, depth);
   fprintf_indent (f, indent,
     "tree _cond_lhs_%d = gimple_cond_lhs (_ct_%d);\n", depth, depth);
   fprintf_indent (f, indent,