Re: [PATCH][Resend][tree-optimization] Fix PR58088

[Richard Earnshaw <rearnsha at arm dot com>]

On 09/09/13 10:56, Kyrylo Tkachov wrote:
> [Resending, since I was away and not pinging it]
> 
> 
> Hi all,
> 
> In PR58088 the constant folder goes into an infinite recursion and runs out of
> stack space because of two conflicting optimisations:
> (X * C1) & C2 plays dirty when nested inside an IOR expression like so: ((X *
> C1) & C2) | C4. One can undo the other leading to an infinite recursion.
> 
> Thanks to Marek for finding the IOR case.
> 
> This patch fixes that by checking in the IOR case that the change to C2 will
> not conflict with the AND case transformation. Example testcases in the PR on
> bugzilla.
> 
> This affects both trunk and 4.8 and regresses and bootstraps cleanly on both.
> 
> Bootstrapped on x86_64-linux-gnu and tested arm-none-eabi on qemu.
> 
> Ok for trunk and 4.8?
> 
> Thanks,
> Kyrill
> 
> 2013-09-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
> 	PR tree-optimization/58088
> 	* fold-const.c (mask_with_trailing_zeros): New function.
> 	(fold_binary_loc): Make sure we don't recurse infinitely
> 	when the X in (X & C1) | C2 is a tree of the form (Y * K1) & K2.
> 	Use mask_with_trailing_zeros where appropriate.
> 	
> 	
> 2013-09-09  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> 
> 	PR tree-optimization/58088
> 	* gcc.c-torture/compile/pr58088.c: New test.=
> 
> 
> pr58088.patch
> 
> 

@@ -9942,6 +9942,22 @@ exact_inverse (tree type, tree cst)
     }
 }

+/*  Mask out the tz least significant bits of X of type TYPE where
+    tz is the number of trailing zeroes in Y.  */
+static double_int
+mask_with_tz (tree type, double_int x, double_int y)
+{
+  int tz = y.trailing_zeros ();
+  if (tz > 0)

blank line between declarations and statements.

@@ -11266,6 +11282,7 @@ fold_binary_loc (location_t loc,
 	{
 	  double_int c1, c2, c3, msk;
 	  int width = TYPE_PRECISION (type), w;
+	  bool valid = true;
 	  c1 = tree_to_double_int (TREE_OPERAND (arg0, 1));
 	  c2 = tree_to_double_int (arg1);

blank line after declarations before code body.

 	    }
-	  if (c3 != c1)
+	  /* If X is a tree of the form (Y * K1) & K2, this might conflict

Should be a blank line before the comment as well

+	     with that optimization from the BIT_AND_EXPR optimizations.
+	     This could end up in an infinite recursion.  */
+	  if (TREE_CODE (TREE_OPERAND (arg0, 0)) == MULT_EXPR
+	      && TREE_CODE (TREE_OPERAND (TREE_OPERAND (arg0, 0), 1))
+	                    == INTEGER_CST)
+	  {
+	    tree t = TREE_OPERAND (TREE_OPERAND (arg0, 0), 1);
+	    double_int masked = mask_with_tz (type, c3, tree_to_double_int (t));
+	    valid = masked != c1;

blank line before statements after declarations.
+	  }
+
+	  if (c3 != c1 && valid)

'valid' should come before the comparison test.  Furthermore, I think
'valid' is misleading; 'try_simplify' would be a more accurate
description of what the test is about.

OK with those changes.

R.