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

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

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

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.  */
       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.