public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Richard Biener <rguenther@suse.de>,
	Andre Vieira <andre.simoesdiasvieira@arm.com>
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] vect: Don't clear base_misaligned in update_epilogue_loop_vinfo [PR114566]
Date: Fri, 5 Apr 2024 12:11:06 +0200	[thread overview]
Message-ID: <Zg/OOohyIo01T6vF@tucnak> (raw)

Hi!

The following testcase is miscompiled, because in the vectorized
epilogue the vectorizer assumes it can use aligned loads/stores
(if the base decl gets alignment increased), but it actually doesn't
increase that.
This is because r10-4203-g97c1460367 added the hunk following
patch removes.  The explanation feels reasonable, but actually it
is not true as the testcase proves.
The thing is, we vectorize the main loop with 64-byte vectors
and the corresponding data refs have base_alignment 16 (the
a array has DECL_ALIGN 128) and offset_alignment 32.  Now, because
of the offset_alignment 32 rather than 64, we need to use unaligned
loads/stores in the main loop (and ditto in the first load/store
in vectorized epilogue).  But the second load/store in the vectorized
epilogue uses only 32-byte vectors and because it is a multiple
of offset_alignment, it checks if we could increase alignment of the
a VAR_DECL, the function returns true, sets base_misaligned = true
and says the access is then aligned.
But when update_epilogue_loop_vinfo clears base_misaligned with the
assumption that the var had to have the alignment increased already,
the update of DECL_ALIGN doesn't happen anymore.

Now, I'd think this base_alignment = false was needed before
r10-4030-gd2db7f7901 change was committed where it incorrectly
overwrote DECL_ALIGN even if it was already larger, rather than
just always increasing it.  But with that change in, it doesn't
make sense to me anymore.

Note, the testcase is latent on the trunk, but reproduces on the 13
branch.

Bootstrapped/regtested on x86_64-linux and i686-linux on the trunk,
plus tested with the testcase on 13 branch with -m32/-m64 without/with
the tree-vect-loop.cc patch (where it FAILed before and now PASSes).
Ok for trunk?

2024-04-05  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/114566
	* tree-vect-loop.cc (update_epilogue_loop_vinfo): Don't clear
	base_misaligned.

	* gcc.target/i386/avx512f-pr114566.c: New test.

--- gcc/tree-vect-loop.cc.jj	2024-04-04 00:48:05.932072711 +0200
+++ gcc/tree-vect-loop.cc	2024-04-05 00:59:33.743101468 +0200
@@ -11590,9 +11590,7 @@ find_in_mapping (tree t, void *context)
    corresponding dr_vec_info need to be reconnected to the EPILOGUE's
    stmt_vec_infos, their statements need to point to their corresponding copy,
    if they are gather loads or scatter stores then their reference needs to be
-   updated to point to its corresponding copy and finally we set
-   'base_misaligned' to false as we have already peeled for alignment in the
-   prologue of the main loop.  */
+   updated to point to its corresponding copy.  */
 
 static void
 update_epilogue_loop_vinfo (class loop *epilogue, tree advance)
@@ -11736,10 +11734,6 @@ update_epilogue_loop_vinfo (class loop *
 	}
       DR_STMT (dr) = STMT_VINFO_STMT (stmt_vinfo);
       stmt_vinfo->dr_aux.stmt = stmt_vinfo;
-      /* The vector size of the epilogue is smaller than that of the main loop
-	 so the alignment is either the same or lower. This means the dr will
-	 thus by definition be aligned.  */
-      STMT_VINFO_DR_INFO (stmt_vinfo)->base_misaligned = false;
     }
 
   epilogue_vinfo->shared->datarefs_copy.release ();
--- gcc/testsuite/gcc.target/i386/avx512f-pr114566.c.jj	2024-04-05 11:21:04.282639386 +0200
+++ gcc/testsuite/gcc.target/i386/avx512f-pr114566.c	2024-04-05 11:21:04.282639386 +0200
@@ -0,0 +1,34 @@
+/* PR tree-optimization/114566 */
+/* { dg-do run } */
+/* { dg-options "-O3 -mavx512f" } */
+/* { dg-additional-options "-fstack-protector-strong" { target fstack_protector } } */
+/* { dg-require-effective-target avx512f } */
+
+#define AVX512F
+#include "avx512f-helper.h"
+
+__attribute__((noipa)) int
+foo (float x, float y)
+{
+  float a[8][56];
+  __builtin_memset (a, 0, sizeof (a));
+
+  for (int j = 0; j < 8; j++)
+    for (int k = 0; k < 56; k++)
+      {
+	float b = k * y;
+	if (b < 0.)
+	  b = 0.;
+	if (b > 0.)
+	  b = 0.;
+	a[j][k] += b;
+      }
+
+  return __builtin_log (x);
+}
+
+void
+TEST (void)
+{
+  foo (86.25f, 0.625f);
+}

	Jakub


             reply	other threads:[~2024-04-05 10:11 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-05 10:11 Jakub Jelinek [this message]
2024-04-05 12:13 ` Richard Biener

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Zg/OOohyIo01T6vF@tucnak \
    --to=jakub@redhat.com \
    --cc=andre.simoesdiasvieira@arm.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=rguenther@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).