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

[Kugan <kugan dot vivekanandarajah at linaro dot org>]

On 26/06/14 11:06, Kugan wrote:
> 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.
> 


I tried SUBREG_PROMOTED_GET () & SRP_UNSIGNED based on what you
suggested (patch attached). the testcases I have for zero/sign extension
are working with this. Bootstrapped and regression tested for
arm-none-linux-gnueabi. If this is OK with you I will do regression
testing on x86_64 and AAcrh64.


Thanks,
Kugan


gcc/
2014-06-26  Kugan Vivekanandarajah  <kuganv@linaro.org>

	* calls.c (precompute_arguments): Use new SUBREG_PROMOTED_SET
	instead of SUBREG_PROMOTED_UNSIGNED_SET
	(expand_call): Likewise.
	* cfgexpand.c (expand_gimple_stmt_1): Use SUBREG_PROMOTED_GET
	& SRP_UNSIGNED to get promoted mode.
	* combine.c (record_promoted_value): Skip > 0 comaprison with
	SUBREG_PROMOTED_UNSIGNED_P as it now returns only 0 or 1.
	* expr.c (convert_move): Use SUBREG_PROMOTED_GET & SRP_UNSIGNED
	instead of SUBREG_PROMOTED_UNSIGNED_P.
	(convert_modes): Likewise.
	(store_expr): Likewise.
	(expand_expr_real_1): Use new SUBREG_PROMOTED_SET
	instead of SUBREG_PROMOTED_UNSIGNED_SET.
	* function.c (assign_parm_setup_reg): Use new SUBREG_PROMOTED_SET
	instead of SUBREG_PROMOTED_UNSIGNED_SET.
	* ifcvt.c (noce_emit_cmove): Updated to use SUBREG_PROMOTED_GET.
	* internal-fn.c (ubsan_expand_si_overflow_mul_check): Use
	SUBREG_PROMOTED_SET instead of SUBREG_PROMOTED_UNSIGNED_SET.
	* optabs.c (widen_operand): Use new SUBREG_PROMOTED_GET
	instead of SUBREG_PROMOTED_UNSIGNED_P.
	* rtl.h (SUBREG_PROMOTED_UNSIGNED_SET): Remove.
	(SUBREG_PROMOTED_SET): New define.
	(SUBREG_PROMOTED_GET): Likewise.
	(SUBREG_PROMOTED_SIGNED_P): Likewise.
	(SUBREG_PROMOTED_UNSIGNED_P): Updated.
	* rtlanal.c (unsigned_reg_p): Use new SUBREG_PROMOTED_GET
	 instead of SUBREG_PROMOTED_UNSIGNED_GET.
	(nonzero_bits1): Skip > 0 comaprison with the results as
	 SUBREG_PROMOTED_UNSIGNED_P now returns only 0 or 1.
	(num_sign_bit_copies1): Use
	SUBREG_PROMOTED_SIGNED_P instead of !SUBREG_PROMOTED_UNSIGNED_P.
	* simplify-rtx.c (simplify_unary_operation_1): Use new
	SUBREG_PROMOTED_SIGNED_P instead of
	!SUBREG_PROMOTED_UNSIGNED_P.
	(simplify_subreg): Use new SUBREG_PROMOTED_SIGNED_P,
	SUBREG_PROMOTED_UNSIGNED_P and SUBREG_PROMOTED_SET instead of
	SUBREG_PROMOTED_UNSIGNED_P and SUBREG_PROMOTED_UNSIGNED_SET.

Attachment: p1.txt
Description: Text document