Re: [PATCH 1/2] Enable setting sign and unsigned promoted mode (SPR_SIGNED_AND_UNSIGNED)

[Kugan <kugan.vivekanandarajah@linaro.org>]

On 25/06/14 17:50, Jakub Jelinek wrote:
> On Wed, Jun 25, 2014 at 05:21:08PM +1000, Kugan wrote:
>> The problem with SRP_POINTER 0, SRP_SIGNED 1, SRP_UNSIGNED 2,
>> SRP_SIGNED_AND_UNSIGNED 3 (as I understand) is that, it will be
>> incompatible with TYPE_UNSIGNED (tree) and defines of
>> POINTER_EXTEND_UNSIGNED values. We will have to then translate while
>> setting to SRP_* values . Also SUBREG_PROMOTED_SIGNED_P is now checked
>> in some cases for != 0 (meaning SRP_POINTER or SRP_UNSIGNED) and in some
>> cases > 0 (meaning SRP_UNSIGNED).
>>
>> Since our aim is to perform single bit checks, why don’t we just use
>> this representation internally (i.e.  _rtx->unchanging = 1 if SRP_SIGNED
>> and _rtx->volatil = 1 if SRP_UNSIGNED). As for SUBREG_PROMOTED_SIGNED_P,
>> we still have to return -1 or 1 depending on SRP_POINTER or SRP_UNSIGNED.
> 
> Why don't you make SUBREG_PROMOTED_UNSIGNED_P just return 0/1 (i.e. the
> single bit), and for places where it would like to match both
> SRP_UNSIGNED and SRP_POINTER use SUBREG_PROMOTED_GET () & SRP_UNSIGNED
> or so?

If we use SUBREG_PROMOTED_GET () & SRP_UNSIGNED, we will miss
the case SRP_SIGNED_AND_UNSIGNED. Though this is not wrong, we might
miss some optimization opportunities here. We can however use
(SUBREG_PROMOTED_GET () != SRP_SIGNED) if you like this. Other option is
to define another macro that explicilty says some think like
SUBREG_PROMOTED_POINTER_OR_UNSIGNED_P.

>> --- a/gcc/ifcvt.c
>> +++ b/gcc/ifcvt.c
>> @@ -1448,8 +1448,11 @@ noce_emit_cmove (struct noce_if_info *if_info, rtx x, enum rtx_code code,
>>  	  || byte_vtrue != byte_vfalse
>>  	  || (SUBREG_PROMOTED_VAR_P (vtrue)
>>  	      != SUBREG_PROMOTED_VAR_P (vfalse))
>> -	  || (SUBREG_PROMOTED_UNSIGNED_P (vtrue)
>> -	      != SUBREG_PROMOTED_UNSIGNED_P (vfalse)))
>> +	  || ((SUBREG_PROMOTED_UNSIGNED_P (vtrue)
>> +	       != SUBREG_PROMOTED_UNSIGNED_P (vfalse))
>> +	      && (SUBREG_PROMOTED_SIGNED_P (vtrue)
>> +		  != SUBREG_PROMOTED_SIGNED_P (vfalse))))
> 
> Shouldn't this be SUBREG_PROMOTED_GET (vtrue) != SUBREG_PROMOTED_GET (vfalse) ?

The reason why I checked like this to cover one side with
SRP_SIGNED_AND_UNSIGNED and other with  SRP_SIGNED or SRP_UNSIGNED. If
we check SUBREG_PROMOTED_GET (vtrue) != SUBREG_PROMOTED_GET (vfalse) we
will miss that.

>> +
>> +/* Predicate to check if RTX of SUBREG_PROMOTED_VAR_P() is promoted
>> +   for UNSIGNED type.  In case of SRP_POINTER, SUBREG_PROMOTED_UNSIGNED_P
>> +   returns -1 as this is in most cases handled like unsigned extension,
>> +   except for generating instructions where special code is emitted for
>> +   (ptr_extend insns) on some architectures.  */
>>  #define SUBREG_PROMOTED_UNSIGNED_P(RTX)	\
>> -  ((RTL_FLAG_CHECK1 ("SUBREG_PROMOTED_UNSIGNED_P", (RTX), SUBREG)->volatil) \
>> -   ? -1 : (int) (RTX)->unchanging)
>> +  ((((RTL_FLAG_CHECK1 ("SUBREG_PROMOTED_UNSIGNED_P", (RTX), SUBREG)->volatil)\
>> +     + (RTX)->unchanging) == 0) ? -1 : ((RTX)->volatil == 1))
>> +
>> +/* Checks if RTX of SUBREG_PROMOTED_VAR_P() is promotd for given SIGN.  */
>> +#define	SUBREG_CHECK_PROMOTED_SIGN(RTX, SIGN) \
> 
> Use space rather than tab.  Also, why do we need this macro?
> Can't you just use SUBREG_PROMOTED_GET () == sign ?  I mean, sign in that
> case is typically just 0 or 1.

Again I wanted to cover SRP_SIGNED_AND_UNSIGNED as well in this case.


Thanks,
Kugan