public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Michael Meissner <meissner@linux.ibm.com>
To: Michael Meissner <meissner@linux.ibm.com>,
	gcc-patches@gcc.gnu.org,
	       Segher Boessenkool <segher@kernel.crashing.org>,
	       David Edelsohn <dje.gcc@gmail.com>
Subject: [PATCH] V11 patch #1 of 15, Fix bug in vec_extract
Date: Fri, 20 Dec 2019 23:28:00 -0000	[thread overview]
Message-ID: <20191220232457.GA28993@ibm-toto.the-meissners.org> (raw)
In-Reply-To: <20191220231507.GA18386@ibm-toto.the-meissners.org>

This patch fixes the bug pointed out in the V10 patch review that the code
modified an input argument to vector extract with a variable element number.

I also added two gcc_asserts to the vector extract address code to signal an
internal error if the temporary base register was used for two different
purposes.  This shows up if you have a vector whose address is a PC-relative
address and the element number was variable.

Later patches will fix the case that I know of that generates the bad code, but
it is still important to make sure the same case doesn't happen in the future.

With this patch applied, the compiler will signal an error.  FWIW, I did build
all of Spec 2017 and Spec 2006 with this patch applied, but not the others, and
we did not get an assertion failure.

I have bootstrapped the compiler and there were no regression test failures on
a little endian Power8 system.

2019-12-20  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.c (rs6000_adjust_vec_address): Add
	assertion to make sure that we don't load an address into a
	temporary that is already used.
	(rs6000_split_vec_extract_var): Do not overwrite the element when
	masking it.  Use the base register temporary instead.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 279549)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -6757,6 +6757,8 @@ rs6000_adjust_vec_address (rtx scalar_re
 
       else
 	{
+	  /* If we are called from rs6000_split_vec_extract_var, base_tmp may
+	     be the same as element.  */
 	  if (TARGET_POWERPC64)
 	    emit_insn (gen_ashldi3 (base_tmp, element, GEN_INT (byte_shift)));
 	  else
@@ -6825,6 +6827,11 @@ rs6000_adjust_vec_address (rtx scalar_re
 
 	  else
 	    {
+	      /* Make sure base_tmp is not the same as element_offset.  This
+		 can happen if the element number is variable and the address
+		 is not a simple address.  Otherwise we lose the offset, and
+		 double the address.  */
+	      gcc_assert (!reg_mentioned_p (base_tmp, element_offset));
 	      emit_move_insn (base_tmp, op1);
 	      emit_insn (gen_add2_insn (base_tmp, element_offset));
 	    }
@@ -6835,6 +6842,10 @@ rs6000_adjust_vec_address (rtx scalar_re
 
   else
     {
+      /* Make sure base_tmp is not the same as element_offset.  This can happen
+	 if the element number is variable and the address is not a simple
+	 address.  Otherwise we lose the offset, and double the address.  */
+      gcc_assert (!reg_mentioned_p (base_tmp, element_offset));
       emit_move_insn (base_tmp, addr);
       new_addr = gen_rtx_PLUS (Pmode, base_tmp, element_offset);
     }
@@ -6902,9 +6913,10 @@ rs6000_split_vec_extract_var (rtx dest,
       int num_elements = GET_MODE_NUNITS (mode);
       rtx num_ele_m1 = GEN_INT (num_elements - 1);
 
-      emit_insn (gen_anddi3 (element, element, num_ele_m1));
+      /* Make sure the element number is in bounds.  */
       gcc_assert (REG_P (tmp_gpr));
-      emit_move_insn (dest, rs6000_adjust_vec_address (dest, src, element,
+      emit_insn (gen_anddi3 (tmp_gpr, element, num_ele_m1));
+      emit_move_insn (dest, rs6000_adjust_vec_address (dest, src, tmp_gpr,
 						       tmp_gpr, scalar_mode));
       return;
     }

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.ibm.com, phone: +1 (978) 899-4797

  reply	other threads:[~2019-12-20 23:25 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-20 23:15 PowerPC -mcpu=future patches, V11 Michael Meissner
2019-12-20 23:28 ` Michael Meissner [this message]
2019-12-22 14:06   ` [PATCH] V11 patch #1 of 15, Fix bug in vec_extract Segher Boessenkool
2019-12-20 23:47 ` [PATCH] V11 patch #2 of 15, Use prefixed load for vector extract with large offset Michael Meissner
2019-12-22 17:24   ` Segher Boessenkool
2020-01-07  1:41     ` [PATCH, committed] " Michael Meissner
2019-12-20 23:49 ` [PATCH] V11 patch #3 of 15, Use 'Q' constraint for variable vector extract from memory Michael Meissner
2019-12-22 17:49   ` Segher Boessenkool
2020-01-07  1:43     ` [PATCH, committed] " Michael Meissner
2019-12-20 23:56 ` [PATCH] V11 patch #4 of 15, Update 'Q' constraint documentation Michael Meissner
2019-12-22 20:02   ` Segher Boessenkool
2020-01-07  1:45     ` [PATCH, committed] " Michael Meissner
2019-12-21  0:00 ` [PATCH] V11 patch #5 of 15, Optimize vec_extract of a vector in memory with a PC-relative address Michael Meissner
2019-12-25  6:41   ` Segher Boessenkool
2020-01-06 20:55     ` Michael Meissner
2020-01-06 21:01     ` Michael Meissner
2020-01-07  1:48     ` [PATCH, committed] " Michael Meissner
2019-12-21  0:03 ` [PATCH] V11 patch #6 of 15, Make -mpcrel the default for -mcpu=future on Linux 64-bit Michael Meissner
2019-12-21  0:06 ` [PATCH] V11 patch #7 of 15, Add new target_supports cases for -mcpu=future tests Michael Meissner
2019-12-21  0:11 ` [PATCH] V11 patch #8 of 15, Add new tests for using PADDI and PLI with -mcpu=future Michael Meissner
2019-12-21  0:12 ` [PATCH] V11 patch #9 of 15, Add test to validate generating prefixed memory when the offset is invalid for DS/DQ insns Michael Meissner
2019-12-21  0:22 ` [PATCH] V11 patch #10 of 15, Make sure we don't generate pre-modify prefixed insns with -mcpu=future Michael Meissner
2019-12-21  0:25 ` [PATCH] V11 patch #12 of 15, Add new PC-relative tests for -mcpu=future Michael Meissner
2019-12-21  0:25 ` [PATCH] V11 patch #11 of 15, Add new tests for generating prefixed loads/stores on -mcpu=future with large offsets Michael Meissner
2019-12-21  0:33 ` [PATCH] V11 patch #13 of 15, Add test for -mcpu=future -fstack-protect-strong with large stacks Michael Meissner
2019-12-21  1:23 ` [PATCH] V11 patch #14 of 15, Add tests for vec_extract from memory with PC-relative addrss Michael Meissner
2019-12-21  1:25 ` [PATCH] V11 patch #15 of 15, Add tests for -mcpu=future vec_extract from memory with a large offset Michael Meissner

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=20191220232457.GA28993@ibm-toto.the-meissners.org \
    --to=meissner@linux.ibm.com \
    --cc=dje.gcc@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=segher@kernel.crashing.org \
    /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).