Thanks Richard for comments.
> This should use gsi_after_labels (bb); otherwise you'll ICE when there's a
> label
> in the BB.
> Please fix the label issue though.
Sure.
> You also have to look out for a first stmt that returns twice since
> you may not insert anything before that. I would suggest to not match when
> BB has abnormal incoming edges which I guess will be ensured by the PHI
> matching code anyway, so I just mentioned this insertion restriction.
Got it, the PHI matching code ensured this.
But I may lose the point about the scenario you mentioned, aka
"a first stmt return twice since you may not insert anything before that".
Could you help to explain more about it? Thanks a lot.
Pan
-----Original Message-----
From: Richard Biener <richard.guent...@gmail.com>
Sent: Tuesday, June 11, 2024 3:07 PM
To: Li, Pan2 <pan2...@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zh...@rivai.ai; kito.ch...@gmail.com
Subject: Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI
match
On Mon, Jun 10, 2024 at 4:49 PM <pan2...@intel.com> wrote:
>
> From: Pan Li <pan2...@intel.com>
>
> When enabled the PHI handing for COND_EXPR, we need to insert the gcall
> to replace the PHI node. Unfortunately, I made a mistake that insert
> the gcall to before the last stmt of the bb. See below gimple, the PHI
> is located at no.1 but we insert the gcall (aka no.9) to the end of
> the bb. Then the use of _9 in no.2 will have no def and will trigger
> ICE when verify_ssa.
>
> 1. # _9 = PHI <_3(4), 18446744073709551615(3)> // The PHI node to be
> deleted.
> 2. prephitmp_36 = (char *) _9;
> 3. buf.write_base = string_13(D);
> 4. buf.write_ptr = string_13(D);
> 5. buf.write_end = prephitmp_36;
> 6. buf.written = 0;
> 7. buf.mode = 3;
> 8. _7 = buf.write_end;
> 9. _9 = .SAT_ADD (string.0_2, maxlen_15(D)); // Insert gcall to last bb
> by mistake
>
> This patch would like to insert the gcall to before the start of the bb
> stmt. To ensure the possible use of PHI_result will have a def exists.
> After this patch the above gimple will be:
>
> 0. _9 = .SAT_ADD (string.0_2, maxlen_15(D)); // Insert gcall to start bb
> by mistake
> 1. # _9 = PHI <_3(4), 18446744073709551615(3)> // The PHI node to be
> deleted.
> 2. prephitmp_36 = (char *) _9;
> 3. buf.write_base = string_13(D);
> 4. buf.write_ptr = string_13(D);
> 5. buf.write_end = prephitmp_36;
> 6. buf.written = 0;
> 7. buf.mode = 3;
> 8. _7 = buf.write_end;
>
> The below test suites are passed for this patch:
> * The rv64gcv fully regression test with newlib.
> * The rv64gcv build with glibc.
> * The x86 regression test with newlib.
> * The x86 bootstrap test with newlib.
>
> PR target/115387
>
> gcc/ChangeLog:
>
> * tree-ssa-math-opts.cc (math_opts_dom_walker::after_dom_children):
> Take
> the gsi of start_bb instead of last_bb.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/pr115387-1.c: New test.
> * gcc.target/riscv/pr115387-2.c: New test.
>
> Signed-off-by: Pan Li <pan2...@intel.com>
> ---
> gcc/testsuite/gcc.target/riscv/pr115387-1.c | 35 +++++++++++++++++++++
> gcc/testsuite/gcc.target/riscv/pr115387-2.c | 18 +++++++++++
> gcc/tree-ssa-math-opts.cc | 2 +-
> 3 files changed, 54 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/pr115387-1.c
> create mode 100644 gcc/testsuite/gcc.target/riscv/pr115387-2.c
>
> diff --git a/gcc/testsuite/gcc.target/riscv/pr115387-1.c
> b/gcc/testsuite/gcc.target/riscv/pr115387-1.c
> new file mode 100644
> index 00000000000..a1c926977c4
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/pr115387-1.c
> @@ -0,0 +1,35 @@
> +/* Test there is no ICE when compile. */
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gcv -mabi=lp64d -O3" } */
> +
> +#define PRINTF_CHK 0x34
> +
> +typedef unsigned long uintptr_t;
> +
> +struct __printf_buffer {
> + char *write_ptr;
> + int status;
> +};
> +
> +extern void __printf_buffer_init_end (struct __printf_buffer *, char *, char
> *);
> +
> +void
> +test (char *string, unsigned long maxlen, unsigned mode_flags)
> +{
> + struct __printf_buffer buf;
> +
> + if ((mode_flags & PRINTF_CHK) != 0)
> + {
> + string[0] = '\0';
> + uintptr_t end;
> +
> + if (__builtin_add_overflow ((uintptr_t) string, maxlen, &end))
> + end = -1;
> +
> + __printf_buffer_init_end (&buf, string, (char *) end);
> + }
> + else
> + __printf_buffer_init_end (&buf, string, (char *) ~(uintptr_t) 0);
> +
> + *buf.write_ptr = '\0';
> +}
> diff --git a/gcc/testsuite/gcc.target/riscv/pr115387-2.c
> b/gcc/testsuite/gcc.target/riscv/pr115387-2.c
> new file mode 100644
> index 00000000000..7183bf18dfd
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/pr115387-2.c
> @@ -0,0 +1,18 @@
> +/* Test there is no ICE when compile. */
> +/* { dg-do compile } */
> +/* { dg-options "-march=rv64gcv -mabi=lp64d -O3" } */
> +
> +#include <stddef.h>
> +#include <stdint-gcc.h>
> +
> +char *
> +test (char *string, size_t maxlen)
> +{
> + string[0] = '\0';
> + uintptr_t end;
> +
> + if (__builtin_add_overflow ((uintptr_t) string, maxlen, &end))
> + end = -1;
> +
> + return (char *) end;
> +}
> diff --git a/gcc/tree-ssa-math-opts.cc b/gcc/tree-ssa-math-opts.cc
> index 173b0366f5e..fbb8e0ea306 100644
> --- a/gcc/tree-ssa-math-opts.cc
> +++ b/gcc/tree-ssa-math-opts.cc
> @@ -6102,7 +6102,7 @@ math_opts_dom_walker::after_dom_children (basic_block
> bb)
> for (gphi_iterator psi = gsi_start_phis (bb); !gsi_end_p (psi);
> gsi_next (&psi))
> {
> - gimple_stmt_iterator gsi = gsi_last_bb (bb);
> + gimple_stmt_iterator gsi = gsi_start_bb (bb);
This should use gsi_after_labels (bb); otherwise you'll ICE when there's a label
in the BB. You also have to look out for a first stmt that returns twice since
you may not insert anything before that. I would suggest to not match when
BB has abnormal incoming edges which I guess will be ensured by the PHI
matching code anyway, so I just mentioned this insertion restriction.
Please fix the label issue though.
Richard.
> match_unsigned_saturation_add (&gsi, psi.phi ());
> }
>
> --
> 2.34.1
>
