diff mbox

[combine] PR middle-end/71074 Check that const_op is >= 0 before potentially shifting in simplify_comparison

Message ID 5735D51A.1050302@foss.arm.com
State New
Headers show

Commit Message

Kyrill Tkachov May 13, 2016, 1:22 p.m. UTC 
On 13/05/16 14:01, Bernd Schmidt wrote:
> On 05/13/2016 02:21 PM, Kyrill Tkachov wrote:
>> Hi all,
>>
>> In this PR we may end up shifting a negative value left in
>> simplify_comparison.
>> The statement is:
>> const_op <<= INTVAL (XEXP (op0, 1));
>>
>> This patch guards the block of that statement by const_op >= 0.
>> I _think_ that's a correct thing to do for that trasnformation:
>> "If we have (compare (xshiftrt FOO N) (const_int C)) and
>>   the low order N bits of FOO are known to be zero, we can do this
>>   by comparing FOO with C shifted left N bits so long as no
>>   overflow occurs."
>
> Isn't the condition testing for overflow though? In a somewhat nonobvious way.
>
> So I think you should just use an unsigned version of const_op. While we're here we might as well make the code here a little more readable. How about the patch below? Can you test whether this works for you?
>

Looks reasonable to me barring some comments below.
I'll test a version of the patch with the comments fixed.

Thanks,
Kyrill

>
> Bernd

        code = simplify_compare_const (code, mode, op0, &op1);
-      const_op = INTVAL (op1);
+      HOST_WIDE_INT const_op = INTVAL (op1);
+      unsigned HOST_WIDE_INT uconst_op = (unsigned HOST_WIDE_INT) const_op;
  
Can this be just "unsigned HOST_WIDE_INT uconst_op = UINTVAL (op1);" ?

...

+	      unsigned HOST_WIDE_INT low_mask
+		= (((unsigned HOST_WIDE_INT) 1 << INTVAL (amount)) - 1);
  	      unsigned HOST_WIDE_INT low_bits
-		= (nonzero_bits (XEXP (op0, 0), mode)
-		   & (((unsigned HOST_WIDE_INT) 1
-		       << INTVAL (XEXP (op0, 1))) - 1));
+		= (nonzero_bits (XEXP (op0, 0), mode) & low_mask);
  	      if (low_bits == 0 || !equality_comparison_p)
  		{

(unsigned HOST_WIDE_INT) 1 can be replaced with HOST_WIDE_INT_1U.

Comments

Bernd Schmidt May 13, 2016, 1:26 p.m. UTC | #1 
On 05/13/2016 03:22 PM, Kyrill Tkachov wrote:
>         /* We only want to handle integral modes.  This catches VOIDmode,
>        CCmode, and the floating-point modes.  An exception is that we
> @@ -11649,7 +11649,8 @@ simplify_comparison (enum rtx_code code,
>         /* Try to simplify the compare to constant, possibly changing the
>        comparison op, and/or changing op1 to zero.  */
>         code = simplify_compare_const (code, mode, op0, &op1);
> -      const_op = INTVAL (op1);
> +      HOST_WIDE_INT const_op = INTVAL (op1);
> +      unsigned HOST_WIDE_INT uconst_op = (unsigned HOST_WIDE_INT)
> const_op;
>
> Can this be just "unsigned HOST_WIDE_INT uconst_op = UINTVAL (op1);" ?

Either should work.

> +          unsigned HOST_WIDE_INT low_mask
> +        = (((unsigned HOST_WIDE_INT) 1 << INTVAL (amount)) - 1);
>             unsigned HOST_WIDE_INT low_bits
> -        = (nonzero_bits (XEXP (op0, 0), mode)
> -           & (((unsigned HOST_WIDE_INT) 1
> -               << INTVAL (XEXP (op0, 1))) - 1));
> +        = (nonzero_bits (XEXP (op0, 0), mode) & low_mask);
>             if (low_bits == 0 || !equality_comparison_p)
>           {
>
> (unsigned HOST_WIDE_INT) 1 can be replaced with HOST_WIDE_INT_1U.

Ah, I suspected there was something like this, but none of the 
surrounding code was using it. Newly changed code should probably use 
that; we could probably improve things further by using it more 
consistently in this function, but let's do that in another patch.


Bernd
diff mbox

Patch

Index: combine.c
===================================================================
--- combine.c	(revision 236113)
+++ combine.c	(working copy)
@@ -11628,13 +11628,13 @@  simplify_comparison (enum rtx_code code,
  
    while (CONST_INT_P (op1))
      {
+      HOST_WIDE_INT amount;


This has to be an rtx rather than HOST_WIDE_INT from the way you use it later on.

  
        /* We only want to handle integral modes.  This catches VOIDmode,
  	 CCmode, and the floating-point modes.  An exception is that we
@@ -11649,7 +11649,8 @@  simplify_comparison (enum rtx_code code,
        /* Try to simplify the compare to constant, possibly changing the
  	 comparison op, and/or changing op1 to zero.  */