Re: Add a load_extend_op wrapper

[Eric Botcazou]

> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?
> 
> Thanks,
> Richard
> 
> 
> gcc/
>   * rtlanal.c (load_extend_op): Move to...
>   * rtl.h: ...here and make inline.

OK, thanks.

-- 
Eric Botcazou

Re: Add a load_extend_op wrapper

[Richard Sandiford]

Eric Botcazou  writes:
>> 2016-11-15  Richard Sandiford  
>>  Alan Hayward  
>>  David Sherwood  
>> 
>>  * rtl.h (load_extend_op): Declare.
>>  * rtlanal.c (load_extend_op): New function.
>
> I'd make it an inline function.

Sorry, I'd committed it before I got this message.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
* rtlanal.c (load_extend_op): Move to...
* rtl.h: ...here and make inline.

diff --git a/gcc/rtl.h b/gcc/rtl.h
index df5172b..b21514f 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -2954,7 +2954,6 @@ extern void set_insn_deleted (rtx);
 
 /* Functions in rtlanal.c */
 
-extern rtx_code load_extend_op (machine_mode);
 extern rtx single_set_2 (const rtx_insn *, const_rtx);
 extern bool contains_symbol_ref_p (const_rtx);
 extern bool contains_symbolic_reference_p (const_rtx);
@@ -3771,5 +3770,17 @@ struct GTY(()) cgraph_rtl_info {
   unsigned function_used_regs_valid: 1;
 };
 
+/* If loads from memories of mode MODE always sign or zero extend,
+   return SIGN_EXTEND or ZERO_EXTEND as appropriate.  Return UNKNOWN
+   otherwise.  */
+
+inline rtx_code
+load_extend_op (machine_mode mode)
+{
+  if (SCALAR_INT_MODE_P (mode)
+  && GET_MODE_PRECISION (mode) < BITS_PER_WORD)
+return LOAD_EXTEND_OP (mode);
+  return UNKNOWN;
+}
 
 #endif /* ! GCC_RTL_H */
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index f07a77a..55a9d2c 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -3863,19 +3863,6 @@ subreg_nregs_with_regno (unsigned int regno, const_rtx x)
   return info.nregs;
 }
 
-/* If loads from memories of mode MODE always sign or zero extend,
-   return SIGN_EXTEND or ZERO_EXTEND as appropriate.  Return UNKNOWN
-   otherwise.  */
-
-rtx_code
-load_extend_op (machine_mode mode)
-{
-  if (SCALAR_INT_MODE_P (mode)
-  && GET_MODE_PRECISION (mode) < BITS_PER_WORD)
-return LOAD_EXTEND_OP (mode);
-  return UNKNOWN;
-}
-
 struct parms_set_data
 {
   int nregs;

Re: Add a load_extend_op wrapper

[Jeff Law]

On 11/15/2016 11:56 AM, Jeff Law wrote:

On 11/15/2016 11:12 AM, Richard Sandiford wrote:

Jeff Law  writes:

On 11/15/2016 05:42 AM, Richard Sandiford wrote:

LOAD_EXTEND_OP only applies to scalar integer modes that are narrower
than a word.  However, callers weren't consistent about which of these
checks they made beforehand, and also weren't consistent about whether
"smaller" was based on (bit)size or precision (IMO it's the latter).
This patch adds a wrapper to try to make the macro easier to use.

It's unclear to me how GET_MODE_PRECISION is different from
GET_MODE_SIZE or GET_MODE_BITSIZE.  But I haven't really thought about
it, particularly in the context of vector modes and such.  I'm certainly
willing to trust your judgment on this.


In this case it's really more about scalar integer modes than vector
modes.
I think using size and precision are equivalent for MODE_INT but they can
be different for MODE_PARTIAL_INT.  Using precision allows LOAD_EXTEND_OP
to apply to (say) PSImode extensions to SImode, whereas using the size
wouldn't, since PSImode and SImode have the same (memory) size.

Ah, partial modes.  No idea what the right thing to do would be.  So
again, I'll trust your judgment.

The only target where I had to deal with partial modes was the mn102. On
that target partial modes (PSI/24 bits) are larger than the machine's
natural word size (16 bits) so LOAD_EXTEND_OP didn't apply.
More correctly, didn't apply to partial modes.  We certainly used it to 
avoid unnecessary extensions when loading 8 bit values.


jeff

Re: Add a load_extend_op wrapper

[Jeff Law]

On 11/15/2016 11:12 AM, Richard Sandiford wrote:

Jeff Law  writes:

On 11/15/2016 05:42 AM, Richard Sandiford wrote:

LOAD_EXTEND_OP only applies to scalar integer modes that are narrower
than a word.  However, callers weren't consistent about which of these
checks they made beforehand, and also weren't consistent about whether
"smaller" was based on (bit)size or precision (IMO it's the latter).
This patch adds a wrapper to try to make the macro easier to use.

It's unclear to me how GET_MODE_PRECISION is different from
GET_MODE_SIZE or GET_MODE_BITSIZE.  But I haven't really thought about
it, particularly in the context of vector modes and such.  I'm certainly
willing to trust your judgment on this.


In this case it's really more about scalar integer modes than vector modes.
I think using size and precision are equivalent for MODE_INT but they can
be different for MODE_PARTIAL_INT.  Using precision allows LOAD_EXTEND_OP
to apply to (say) PSImode extensions to SImode, whereas using the size
wouldn't, since PSImode and SImode have the same (memory) size.
Ah, partial modes.  No idea what the right thing to do would be.  So 
again, I'll trust your judgment.


The only target where I had to deal with partial modes was the mn102. 
On that target partial modes (PSI/24 bits) are larger than the machine's 
natural word size (16 bits) so LOAD_EXTEND_OP didn't apply.


jeff

Re: Add a load_extend_op wrapper

[Eric Botcazou]

> 2016-11-15  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
> 
>   * rtl.h (load_extend_op): Declare.
>   * rtlanal.c (load_extend_op): New function.

I'd make it an inline function.

-- 
Eric Botcazou

Re: Add a load_extend_op wrapper

[Richard Sandiford]

Jeff Law  writes:
> On 11/15/2016 05:42 AM, Richard Sandiford wrote:
>> LOAD_EXTEND_OP only applies to scalar integer modes that are narrower
>> than a word.  However, callers weren't consistent about which of these
>> checks they made beforehand, and also weren't consistent about whether
>> "smaller" was based on (bit)size or precision (IMO it's the latter).
>> This patch adds a wrapper to try to make the macro easier to use.
> It's unclear to me how GET_MODE_PRECISION is different from 
> GET_MODE_SIZE or GET_MODE_BITSIZE.  But I haven't really thought about 
> it, particularly in the context of vector modes and such.  I'm certainly 
> willing to trust your judgment on this.

In this case it's really more about scalar integer modes than vector modes.
I think using size and precision are equivalent for MODE_INT but they can
be different for MODE_PARTIAL_INT.  Using precision allows LOAD_EXTEND_OP
to apply to (say) PSImode extensions to SImode, whereas using the size
wouldn't, since PSImode and SImode have the same (memory) size.

>> The patch doesn't change reload, since different checks could have
>> unforeseen consequences.
> I think the same concepts apply in reload, but I understand the 
> hesitation to twiddle that code and deal with possible fallout.

Yeah :-)  I know it's a bit of a cop-out, but given the scale of the SVE
changes as a whole, we didn't want to go looking for unnecessary trouble.

>> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?
>>
>> Thanks,
>> Richard
>>
>>
>> [ This patch is part of the SVE series posted here:
>>   https://gcc.gnu.org/ml/gcc/2016-11/msg00030.html ]
>>
>> gcc/
>> 2016-11-15  Richard Sandiford  
>>  Alan Hayward  
>>  David Sherwood  
>>
>>  * rtl.h (load_extend_op): Declare.
>>  * rtlanal.c (load_extend_op): New function.
>>  (nonzero_bits1): Use it.
>>  (num_sign_bit_copies1): Likewise.
>>  * cse.c (cse_insn): Likewise.
>>  * fold-const.c (fold_single_bit_test): Likewise.
>>  (fold_unary_loc): Likewise.
>>  * fwprop.c (free_load_extend): Likewise.
>>  * postreload.c (reload_cse_simplify_set): Likewise.
>>  (reload_cse_simplify_operands): Likewise.
>>  * combine.c (try_combine): Likewise.
>>  (simplify_set): Likewise.  Remove redundant SUBREG_BYTE and
>>  subreg_lowpart_p checks.
> OK.
> jeff

Thanks,
Richard

Re: Add a load_extend_op wrapper

[Jeff Law]

On 11/15/2016 05:42 AM, Richard Sandiford wrote:

LOAD_EXTEND_OP only applies to scalar integer modes that are narrower
than a word.  However, callers weren't consistent about which of these
checks they made beforehand, and also weren't consistent about whether
"smaller" was based on (bit)size or precision (IMO it's the latter).
This patch adds a wrapper to try to make the macro easier to use.
It's unclear to me how GET_MODE_PRECISION is different from 
GET_MODE_SIZE or GET_MODE_BITSIZE.  But I haven't really thought about 
it, particularly in the context of vector modes and such.  I'm certainly 
willing to trust your judgment on this.






LOAD_EXTEND_OP is often used to disable transformations that aren't
beneficial when extends from memory are free, so being stricter about
the check accidentally exposed more optimisation opportunities.

Right.



"SUBREG_BYTE (...) == 0" and subreg_lowpart_p are implied by
paradoxical_subreg_p, so the patch also removes some redundant tests.

Always helpful.



The patch doesn't change reload, since different checks could have
unforeseen consequences.
I think the same concepts apply in reload, but I understand the 
hesitation to twiddle that code and deal with possible fallout.




Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Thanks,
Richard


[ This patch is part of the SVE series posted here:
  https://gcc.gnu.org/ml/gcc/2016-11/msg00030.html ]

gcc/
2016-11-15  Richard Sandiford  
Alan Hayward  
David Sherwood  

* rtl.h (load_extend_op): Declare.
* rtlanal.c (load_extend_op): New function.
(nonzero_bits1): Use it.
(num_sign_bit_copies1): Likewise.
* cse.c (cse_insn): Likewise.
* fold-const.c (fold_single_bit_test): Likewise.
(fold_unary_loc): Likewise.
* fwprop.c (free_load_extend): Likewise.
* postreload.c (reload_cse_simplify_set): Likewise.
(reload_cse_simplify_operands): Likewise.
* combine.c (try_combine): Likewise.
(simplify_set): Likewise.  Remove redundant SUBREG_BYTE and
subreg_lowpart_p checks.

OK.
jeff