[combine] Don't create vector mode ZERO_EXTEND from subregs

Message ID 1513001933-17348-1-git-send-email-james.greenhalgh@arm.com
State New
Headers show
Series
  • [combine] Don't create vector mode ZERO_EXTEND from subregs
Related show

Commit Message

James Greenhalgh Dec. 11, 2017, 2:18 p.m.
Hi,

In simplify_set we try transforming the paradoxical subreg expression:

  (set FOO (subreg:M (mem:N BAR) 0))

in to:

  (set FOO (zero_extend:M (mem:N BAR)))

However, this code does not consider the case where M is a vector
mode, allowing it to construct (for example):

  (zero_extend:V4SI (mem:SI))

This would clearly have the wrong semantics, but fortunately we fail long
before then in expand_compound_operation. As we really don't want a vector
zero_extend of a scalar value.

We need to explicitly reject vector modes from this transformation.

This fixes a failure I'm seeing on a branch in which I'm trying to
tackle some performance regressions, so I have no live testcase for
this, but it is wrong by observation.

Tested on aarch64-none-elf and bootstrapped on aarch64-none-linux-gnu with
no issues.

OK?

Thanks,
James

---
2017-12-11  James Greenhalgh  <james.greenhalgh@arm.com>

	* combine.c (simplify_set): Do not transform subregs to zero_extends
	if the destination mode is a vector mode.

Comments

Jeff Law Dec. 11, 2017, 9:29 p.m. | #1
On 12/11/2017 07:18 AM, James Greenhalgh wrote:
> 

> Hi,

> 

> In simplify_set we try transforming the paradoxical subreg expression:

> 

>   (set FOO (subreg:M (mem:N BAR) 0))

> 

> in to:

> 

>   (set FOO (zero_extend:M (mem:N BAR)))

> 

> However, this code does not consider the case where M is a vector

> mode, allowing it to construct (for example):

> 

>   (zero_extend:V4SI (mem:SI))

> 

> This would clearly have the wrong semantics, but fortunately we fail long

> before then in expand_compound_operation. As we really don't want a vector

> zero_extend of a scalar value.

> 

> We need to explicitly reject vector modes from this transformation.

> 

> This fixes a failure I'm seeing on a branch in which I'm trying to

> tackle some performance regressions, so I have no live testcase for

> this, but it is wrong by observation.

> 

> Tested on aarch64-none-elf and bootstrapped on aarch64-none-linux-gnu with

> no issues.

> 

> OK?

> 

> Thanks,

> James

> 

> ---

> 2017-12-11  James Greenhalgh  <james.greenhalgh@arm.com>

> 

> 	* combine.c (simplify_set): Do not transform subregs to zero_extends

> 	if the destination mode is a vector mode.

> 

OK.  Ideally you'd have a test for the testsuite as well, but I won't
stress without it :-)


jeff
Marc Glisse Dec. 16, 2017, 2:04 p.m. | #2
I was happily going to close PR 55549, but it looks like we are trying the 
same transformation in a second place in that file :-(

On Mon, 11 Dec 2017, James Greenhalgh wrote:

> Hi,

>

> In simplify_set we try transforming the paradoxical subreg expression:

>

>  (set FOO (subreg:M (mem:N BAR) 0))

>

> in to:

>

>  (set FOO (zero_extend:M (mem:N BAR)))

>

> However, this code does not consider the case where M is a vector

> mode, allowing it to construct (for example):

>

>  (zero_extend:V4SI (mem:SI))

>

> This would clearly have the wrong semantics, but fortunately we fail long

> before then in expand_compound_operation. As we really don't want a vector

> zero_extend of a scalar value.

>

> We need to explicitly reject vector modes from this transformation.

>

> This fixes a failure I'm seeing on a branch in which I'm trying to

> tackle some performance regressions, so I have no live testcase for

> this, but it is wrong by observation.

>

> Tested on aarch64-none-elf and bootstrapped on aarch64-none-linux-gnu with

> no issues.

>

> OK?

>

> Thanks,

> James

>

> ---

> 2017-12-11  James Greenhalgh  <james.greenhalgh@arm.com>

>

> 	* combine.c (simplify_set): Do not transform subregs to zero_extends

> 	if the destination mode is a vector mode.

>

>


-- 
Marc Glisse
Segher Boessenkool Dec. 17, 2017, 3:14 a.m. | #3
Hi!

On Mon, Dec 11, 2017 at 02:18:53PM +0000, James Greenhalgh wrote:
> 

> In simplify_set we try transforming the paradoxical subreg expression:

> 

>   (set FOO (subreg:M (mem:N BAR) 0))

> 

> in to:

> 

>   (set FOO (zero_extend:M (mem:N BAR)))

> 

> However, this code does not consider the case where M is a vector

> mode, allowing it to construct (for example):

> 

>   (zero_extend:V4SI (mem:SI))

> 

> This would clearly have the wrong semantics, but fortunately we fail long

> before then in expand_compound_operation. As we really don't want a vector

> zero_extend of a scalar value.

> 

> We need to explicitly reject vector modes from this transformation.


It does not consider any other modes either.  Both modes involved are
required to be SCALAR_INT_MODE_P (for zero_extend to be valid at all and
to be equivalent to the subreg); could you test for that instead please?


Segher
James Greenhalgh Dec. 21, 2017, 4:34 p.m. | #4
On Sun, Dec 17, 2017 at 03:14:08AM +0000, Segher Boessenkool wrote:
> Hi!

>

> On Mon, Dec 11, 2017 at 02:18:53PM +0000, James Greenhalgh wrote:

> >

> > In simplify_set we try transforming the paradoxical subreg expression:

> >

> >   (set FOO (subreg:M (mem:N BAR) 0))

> >

> > in to:

> >

> >   (set FOO (zero_extend:M (mem:N BAR)))

> >

> > However, this code does not consider the case where M is a vector

> > mode, allowing it to construct (for example):

> >

> >   (zero_extend:V4SI (mem:SI))

> >

> > This would clearly have the wrong semantics, but fortunately we fail long

> > before then in expand_compound_operation. As we really don't want a vector

> > zero_extend of a scalar value.

> >

> > We need to explicitly reject vector modes from this transformation.

>

> It does not consider any other modes either.  Both modes involved are

> required to be SCALAR_INT_MODE_P (for zero_extend to be valid at all and

> to be equivalent to the subreg); could you test for that instead please?


Makes sense. I've committed this as obvious after an AArch64 bootstrap and
test as revision 255945.

Thanks,
James

---
gcc/

2017-12-21  James Greenhalgh  <james.greenhalgh@arm.com>

	* combine.c (simplify_set): Do not transform subregs to zero_extends
	if the destination is not a scalar int mode.
diff --git a/gcc/combine.c b/gcc/combine.c
index 9f19ee4..91bed06 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -6967,12 +6967,13 @@ simplify_set (rtx x)
   /* If we have (set FOO (subreg:M (mem:N BAR) 0)) with M wider than N, this
      would require a paradoxical subreg.  Replace the subreg with a
      zero_extend to avoid the reload that would otherwise be required.
-     Don't do this for vector modes, as the transformation is incorrect.  */
+     Don't do this unless we have a scalar integer mode, otherwise the
+     transformation is incorrect.  */
 
   enum rtx_code extend_op;
   if (paradoxical_subreg_p (src)
       && MEM_P (SUBREG_REG (src))
-      && !VECTOR_MODE_P (GET_MODE (src))
+      && SCALAR_INT_MODE_P (GET_MODE (src))
       && (extend_op = load_extend_op (GET_MODE (SUBREG_REG (src)))) != UNKNOWN)
     {
       SUBST (SET_SRC (x),

Patch

diff --git a/gcc/combine.c b/gcc/combine.c
index 786a840..562eae6 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -6962,11 +6962,13 @@  simplify_set (rtx x)
 
   /* If we have (set FOO (subreg:M (mem:N BAR) 0)) with M wider than N, this
      would require a paradoxical subreg.  Replace the subreg with a
-     zero_extend to avoid the reload that would otherwise be required.  */
+     zero_extend to avoid the reload that would otherwise be required.
+     Don't do this for vector modes, as the transformation is incorrect.  */
 
   enum rtx_code extend_op;
   if (paradoxical_subreg_p (src)
       && MEM_P (SUBREG_REG (src))
+      && !VECTOR_MODE_P (GET_MODE (src))
       && (extend_op = load_extend_op (GET_MODE (SUBREG_REG (src)))) != UNKNOWN)
     {
       SUBST (SET_SRC (x),