diff mbox series

Restrict vector use of extract_bit_field_as_subreg (PR 83699)

Message ID 87h8rzgjd8.fsf@linaro.org
State New
Headers show
Series Restrict vector use of extract_bit_field_as_subreg (PR 83699) | expand

Commit Message

Richard Sandiford Jan. 6, 2018, 2:19 p.m. UTC
The code in r256209 tried using subregs for two cases: to extract
a lowpart element of a vector, or to extract a subvector from a
wider vector.  An earlier version of the SVE port used both variants,
but these days SVE provides vec_extract* for all cases and so only
really needs the subvector extract, e.g. for extracting one vector
from an LD[234] result.

64-bit SPARC is unusual in that REGMODE_NATURAL_SIZE for vectors is
smaller than for GPRs.  It's therefore valid to form a subreg reference
to both halves of a V2SI but not to both halves of a DI.  This is where
the problem in PR83699 comes from: we extract the highpart SI from a V2SI
and want to move the result into a GPR.  This causes LRA to cycle,
because it keeps emitting reload sequences of the form:

   (set (reg:SI tmp) (subreg:SI (reg:V2SI foo) 0))
   (set (reg:SI reg) (reg:SI tmp))

but then assigning a GPR to tmp, thus creating the same reload
problem as before.

However, even with that fixed, the sequence is worse than it was
before r256209.  I don't think this means that using subregs is
a bad idea in principle, just that it needs a different condition.
(E.g. if SPARC supported V2SF, or if it was prepared to do the SImode
operation on FPRs, the patch would have been an improvement, because
we'd avoid doing the extraction via memory.)

But since there's no longer a motivating example for the single-element
case, I think the best approach is just to drop it.  For the remaining
subvector case, we *could* continue to use the GET_MODE_INNER check
as well, but I don't think it's necessary, since what really matters
is whether the register reference can be formed.

Tested so far on aarch64-linux-gnu, and by spot-checking a
sparch64-linux-gnu cross.  OK To install?

Richard


2018-01-06  Richard Sandiford  <richard.sandiford@linaro.org>

gcc/
	PR rtl-optimization/83699
	* expmed.c (extract_bit_field_1): Restrict the vector usage of
	extract_bit_field_as_subreg to cases in which the extracted
	value is also a vector.

Comments

Jeff Law Jan. 7, 2018, 4 a.m. UTC | #1
On 01/06/2018 07:19 AM, Richard Sandiford wrote:
> The code in r256209 tried using subregs for two cases: to extract

> a lowpart element of a vector, or to extract a subvector from a

> wider vector.  An earlier version of the SVE port used both variants,

> but these days SVE provides vec_extract* for all cases and so only

> really needs the subvector extract, e.g. for extracting one vector

> from an LD[234] result.

> 

> 64-bit SPARC is unusual in that REGMODE_NATURAL_SIZE for vectors is

> smaller than for GPRs.  It's therefore valid to form a subreg reference

> to both halves of a V2SI but not to both halves of a DI.  This is where

> the problem in PR83699 comes from: we extract the highpart SI from a V2SI

> and want to move the result into a GPR.  This causes LRA to cycle,

> because it keeps emitting reload sequences of the form:

> 

>    (set (reg:SI tmp) (subreg:SI (reg:V2SI foo) 0))

>    (set (reg:SI reg) (reg:SI tmp))

> 

> but then assigning a GPR to tmp, thus creating the same reload

> problem as before.

> 

> However, even with that fixed, the sequence is worse than it was

> before r256209.  I don't think this means that using subregs is

> a bad idea in principle, just that it needs a different condition.

> (E.g. if SPARC supported V2SF, or if it was prepared to do the SImode

> operation on FPRs, the patch would have been an improvement, because

> we'd avoid doing the extraction via memory.)

> 

> But since there's no longer a motivating example for the single-element

> case, I think the best approach is just to drop it.  For the remaining

> subvector case, we *could* continue to use the GET_MODE_INNER check

> as well, but I don't think it's necessary, since what really matters

> is whether the register reference can be formed.

> 

> Tested so far on aarch64-linux-gnu, and by spot-checking a

> sparch64-linux-gnu cross.  OK To install?

> 

> Richard

> 

> 

> 2018-01-06  Richard Sandiford  <richard.sandiford@linaro.org>

> 

> gcc/

> 	PR rtl-optimization/83699

> 	* expmed.c (extract_bit_field_1): Restrict the vector usage of

> 	extract_bit_field_as_subreg to cases in which the extracted

> 	value is also a vector.

THanks.  Installed.

jeff
diff mbox series

Patch

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	2018-01-06 10:55:43.333837784 +0000
+++ gcc/expmed.c	2018-01-06 14:15:01.179591726 +0000
@@ -1738,16 +1738,10 @@  extract_bit_field_1 (rtx str_rtx, poly_u
 	      return target;
 	    }
 	}
-      /* Using subregs is useful if we're extracting the least-significant
-	 vector element, or if we're extracting one register vector from
-	 a multi-register vector.  extract_bit_field_as_subreg checks
-	 for valid bitsize and bitnum, so we don't need to do that here.
-
-	 The mode check makes sure that we're extracting either
-	 a single element or a subvector with the same element type.
-	 If the modes aren't such a natural fit, fall through and
-	 bitcast to integers first.  */
-      if (GET_MODE_INNER (mode) == innermode)
+      /* Using subregs is useful if we're extracting one register vector
+	 from a multi-register vector.  extract_bit_field_as_subreg checks
+	 for valid bitsize and bitnum, so we don't need to do that here.  */
+      if (VECTOR_MODE_P (mode))
 	{
 	  rtx sub = extract_bit_field_as_subreg (mode, op0, bitsize, bitnum);
 	  if (sub)