[PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[pan2.li]

From: Pan Li <pan2.li@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.li@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);
       match_unsigned_saturation_add (&gsi, psi.phi ());
     }
 
-- 
2.34.1

Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Sam James]

[-- Attachment #1: Type: text/plain, Size: 4844 bytes --]

pan2.li@intel.com writes:

> From: Pan Li <pan2.li@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.li@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;
> +}

This testcases ICEs for me on x86-64 too (without your patch) with just -O2.

Can you move it out of the riscv suite? (I suspect the other fails on x86-64 too).

> 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);
>        match_unsigned_saturation_add (&gsi, psi.phi ());
>      }

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 377 bytes --]

Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Jeff Law]

On 6/10/24 8:49 AM, pan2.li@intel.com wrote:
> From: Pan Li <pan2.li@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.
So the patch looks fine.  I'm just trying to parse the testing.  If you 
did an x86 bootstrap & regression test, you wouldn't be using newlib. 
That would be a native bootstrap & regression test which would use 
whatever C library is already installed on the system.  I'm assuming 
that's what you did.

If my assumption is correct, then this is fine for the trunk.

jeff

Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Jeff Law]

On 6/10/24 8:49 AM, pan2.li@intel.com wrote:
> 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.
I did a fresh x86_64 bootstrap and regression test and pushed this.

jeff

RE: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Li, Pan2]

Thank a lot, Jeff.

Pan

-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com> 
Sent: Tuesday, June 11, 2024 4:15 AM
To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com
Subject: Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match



On 6/10/24 8:49 AM, pan2.li@intel.com wrote:
> 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.
I did a fresh x86_64 bootstrap and regression test and pushed this.

jeff

RE: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Li, Pan2]

Hi Sam,

> This testcases ICEs for me on x86-64 too (without your patch) with just -O2.
> Can you move it out of the riscv suite? (I suspect the other fails on x86-64 too).

Sure thing, but do you have any suggestion about where should I put these 2 cases? 
There are sorts of sub-directories under gcc/testsuite, I am not very familiar that where
is the best reasonable location.

Pan

-----Original Message-----
From: Sam James <sam@gentoo.org> 
Sent: Monday, June 10, 2024 11:33 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com
Subject: Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

pan2.li@intel.com writes:

> From: Pan Li <pan2.li@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.li@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;
> +}

This testcases ICEs for me on x86-64 too (without your patch) with just -O2.

Can you move it out of the riscv suite? (I suspect the other fails on x86-64 too).

> 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);
>        match_unsigned_saturation_add (&gsi, psi.phi ());
>      }

Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Jeff Law]

On 6/10/24 7:28 PM, Li, Pan2 wrote:
> Hi Sam,
> 
>> This testcases ICEs for me on x86-64 too (without your patch) with just -O2.
>> Can you move it out of the riscv suite? (I suspect the other fails on x86-64 too).
> 
> Sure thing, but do you have any suggestion about where should I put these 2 cases?
> There are sorts of sub-directories under gcc/testsuite, I am not very familiar that where
> is the best reasonable location.
gcc.dg/torture would be the most natural location I think.

jeff

RE: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Li, Pan2]

Got it, thanks. Let me prepare the patch after test.

Pan

-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com> 
Sent: Tuesday, June 11, 2024 9:42 AM
To: Li, Pan2 <pan2.li@intel.com>; Sam James <sam@gentoo.org>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; richard.guenther@gmail.com
Subject: Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match



On 6/10/24 7:28 PM, Li, Pan2 wrote:
> Hi Sam,
> 
>> This testcases ICEs for me on x86-64 too (without your patch) with just -O2.
>> Can you move it out of the riscv suite? (I suspect the other fails on x86-64 too).
> 
> Sure thing, but do you have any suggestion about where should I put these 2 cases?
> There are sorts of sub-directories under gcc/testsuite, I am not very familiar that where
> is the best reasonable location.
gcc.dg/torture would be the most natural location I think.

jeff

Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Richard Biener]

On Mon, Jun 10, 2024 at 4:49 PM <pan2.li@intel.com> wrote:
>
> From: Pan Li <pan2.li@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.li@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
>

RE: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Li, Pan2]

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.guenther@gmail.com> 
Sent: Tuesday, June 11, 2024 3:07 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@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.li@intel.com> wrote:
>
> From: Pan Li <pan2.li@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.li@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
>

Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Richard Biener]

On Tue, Jun 11, 2024 at 9:45 AM Li, Pan2 <pan2.li@intel.com> wrote:
>
> 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.

When we have a setjmp call an incoming abnormal edge represents the
alternate return from the call from any point that can do longjmp.  Inserting
before the call would be inserting on that edge which cannot be done.
It's a bit of an awkward representation since in reality we'd have to split
the call into the actual call and the longjmp receiver.

Richard.

>
> Pan
>
> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Tuesday, June 11, 2024 3:07 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@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.li@intel.com> wrote:
> >
> > From: Pan Li <pan2.li@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.li@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
> >

RE: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

[Li, Pan2]

Got it. Thanks Richard.

Pan

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Tuesday, June 11, 2024 5:31 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@gmail.com
Subject: Re: [PATCH v1] Widening-Mul: Fix one ICE of gcall insertion for PHI match

On Tue, Jun 11, 2024 at 9:45 AM Li, Pan2 <pan2.li@intel.com> wrote:
>
> 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.

When we have a setjmp call an incoming abnormal edge represents the
alternate return from the call from any point that can do longjmp.  Inserting
before the call would be inserting on that edge which cannot be done.
It's a bit of an awkward representation since in reality we'd have to split
the call into the actual call and the longjmp receiver.

Richard.

>
> Pan
>
> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Tuesday, June 11, 2024 3:07 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@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.li@intel.com> wrote:
> >
> > From: Pan Li <pan2.li@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.li@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
> >