RE: [PATCH] Alternate fix to PR target/96558: Robustify ix86_expand_clear.

RE: [PATCH] Alternate fix to PR target/96558: Robustify ix86_expand_clear.

[Roger Sayle]

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


Doh! ENOPATCH.

-----Original Message-----
From: Roger Sayle <roger@nextmovesoftware.com> 
Sent: 13 August 2020 09:29
To: 'GCC Patches' <gcc-patches@gcc.gnu.org>
Cc: 'Uros Bizjak' <ubizjak@gmail.com>
Subject: [PATCH] Alternate fix to PR target/96558: Robustify
ix86_expand_clear.


This patch is an alternate/supplementary fix to the recent regression PR
target/96558.  Currently ix86_expand_clear may/should only be called with a
general register DEST after reload_completed.  With the simple change below,
this function now checks these conditions itself, and does the right thing
(or at least something reasonable) rather than ICE.

This change alone is sufficient to fix the recent regression, and allow the
recently added testcase to pass, but following the "why fix something just
once" maxim, I propose adding both solutions (to reduce the risk of
surprises in the future).  Leaving the peephole2 fix in place is reasonable,
as scheduling or a later pass eventually move the condition code setter/use
next to each other, so moving the vpxor with the
peephole2 provides no (additional) benefit.  i.e. gcc.dg/pr96558.c contains:

        vpxor   %xmm0, %xmm0, %xmm0
        testl   %eax, %eax
        jne     .L91

This patch has been tested on x86_64-pc-linux-gnu (without the peephole2
solution) with a make bootstrap and make -k check with no new failures, but
allows the recently added testcase to pass.

Ok for mainline?


2020-08-19  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog
	PR target/96558
	* config/i386/i386-expand.c (ix86_expand_clear): Explicitly
	test for reload_completed and GENERAL_REG_P, and emit a simple
	set of const0_rtx otherwise.

Thanks in advance.
Sorry for the inconvenience.
Roger
--
Roger Sayle
NextMove Software
Cambridge, UK


[-- Attachment #2: patchr2a.txt --]
[-- Type: text/plain, Size: 1346 bytes --]

diff --git a/gcc/config/i386/i386-expand.c b/gcc/config/i386/i386-expand.c
index f441ba9..cf3e741 100644
--- a/gcc/config/i386/i386-expand.c
+++ b/gcc/config/i386/i386-expand.c
@@ -164,18 +164,22 @@ ix86_expand_clear (rtx dest)
   rtx tmp;
 
   /* We play register width games, which are only valid after reload.  */
-  gcc_assert (reload_completed);
-
-  /* Avoid HImode and its attendant prefix byte.  */
-  if (GET_MODE_SIZE (GET_MODE (dest)) < 4)
-    dest = gen_rtx_REG (SImode, REGNO (dest));
-  tmp = gen_rtx_SET (dest, const0_rtx);
-
-  if (!TARGET_USE_MOV0 || optimize_insn_for_size_p ())
+  if (reload_completed && GENERAL_REG_P (dest))
     {
-      rtx clob = gen_rtx_CLOBBER (VOIDmode, gen_rtx_REG (CCmode, FLAGS_REG));
-      tmp = gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, tmp, clob));
+      /* Avoid HImode and its attendant prefix byte.  */
+      if (GET_MODE_SIZE (GET_MODE (dest)) < 4)
+        dest = gen_rtx_REG (SImode, REGNO (dest));
+      tmp = gen_rtx_SET (dest, const0_rtx);
+
+      if (!TARGET_USE_MOV0 || optimize_insn_for_size_p ())
+	{
+	  rtx clob = gen_rtx_CLOBBER (VOIDmode,
+				      gen_rtx_REG (CCmode, FLAGS_REG));
+	  tmp = gen_rtx_PARALLEL (VOIDmode, gen_rtvec (2, tmp, clob));
+	}
     }
+  else
+    tmp = gen_rtx_SET (dest, const0_rtx);
 
   emit_insn (tmp);
 }

Re: [PATCH] Alternate fix to PR target/96558: Robustify ix86_expand_clear.

[Uros Bizjak]

On Thu, Aug 13, 2020 at 10:28 AM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> This patch is an alternate/supplementary fix to the recent regression
> PR target/96558.  Currently ix86_expand_clear may/should only be called
> with a general register DEST after reload_completed.  With the simple
> change below, this function now checks these conditions itself, and
> does the right thing (or at least something reasonable) rather than ICE.
>
> This change alone is sufficient to fix the recent regression, and allow
> the recently added testcase to pass, but following the "why fix something
> just once" maxim, I propose adding both solutions (to reduce the risk
> of surprises in the future).  Leaving the peephole2 fix in place is
> reasonable, as scheduling or a later pass eventually move the condition
> code setter/use next to each other, so moving the vpxor with the
> peephole2 provides no (additional) benefit.  i.e. gcc.dg/pr96558.c contains:
>
>         vpxor   %xmm0, %xmm0, %xmm0
>         testl   %eax, %eax
>         jne     .L91
>
> This patch has been tested on x86_64-pc-linux-gnu (without the peephole2
> solution) with a make bootstrap and make -k check with no new failures,
> but allows the recently added testcase to pass.


But we gain nothing for non-GENERAL_REG_P registers. movdi/movsi
patterns can load 0 to XMM registers with pxor by itself, also with
mask registers. The reason that this peephole exists is due to the
fact that XOR with GENERAL_REGs clobbers flags, so we can't just stick
 "xor reg, reg" to movdi/movsi pattern when zero is loaded. We would
like to do so, but we can't - so we have to complicate our lives with
a peephole pattern that checks for live FLAGS_REG before
transformation.

I think that we should assert that only GENERAL_REGS should be
processed by ix86_expand_clear.

Uros.

> Ok for mainline?
>
>
> 2020-08-19  Roger Sayle  <roger@nextmovesoftware.com>
>
> gcc/ChangeLog
>         PR target/96558
>         * config/i386/i386-expand.c (ix86_expand_clear): Explicitly
>         test for reload_completed and GENERAL_REG_P, and emit a simple
>         set of const0_rtx otherwise.
>
> Thanks in advance.
> Sorry for the inconvenience.
> Roger
> --
> Roger Sayle
> NextMove Software
> Cambridge, UK
>
>

[PATCH] Alternate fix to PR target/96558: Robustify ix86_expand_clear.

[Roger Sayle]

This patch is an alternate/supplementary fix to the recent regression
PR target/96558.  Currently ix86_expand_clear may/should only be called
with a general register DEST after reload_completed.  With the simple
change below, this function now checks these conditions itself, and
does the right thing (or at least something reasonable) rather than ICE.

This change alone is sufficient to fix the recent regression, and allow
the recently added testcase to pass, but following the "why fix something
just once" maxim, I propose adding both solutions (to reduce the risk
of surprises in the future).  Leaving the peephole2 fix in place is
reasonable, as scheduling or a later pass eventually move the condition
code setter/use next to each other, so moving the vpxor with the
peephole2 provides no (additional) benefit.  i.e. gcc.dg/pr96558.c contains:

        vpxor   %xmm0, %xmm0, %xmm0
        testl   %eax, %eax
        jne     .L91

This patch has been tested on x86_64-pc-linux-gnu (without the peephole2
solution) with a make bootstrap and make -k check with no new failures,
but allows the recently added testcase to pass.

Ok for mainline?


2020-08-19  Roger Sayle  <roger@nextmovesoftware.com>

gcc/ChangeLog
	PR target/96558
	* config/i386/i386-expand.c (ix86_expand_clear): Explicitly
	test for reload_completed and GENERAL_REG_P, and emit a simple
	set of const0_rtx otherwise.

Thanks in advance.
Sorry for the inconvenience.
Roger
--
Roger Sayle
NextMove Software
Cambridge, UK