This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [PATCH] Reassociate X == CST1 || X == CST2 if popcount (CST2 - CST1) == 1 into ((X - CST1) & ~(CST2 - CST1)) == 0



> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Monday, October 14, 2013 4:49 PM
> To: Zhenqiang Chen
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] Reassociate X == CST1 || X == CST2 if popcount (CST2
-
> CST1) == 1 into ((X - CST1) & ~(CST2 - CST1)) == 0
> 
> On Mon, Oct 14, 2013 at 03:10:12PM +0800, Zhenqiang Chen wrote:
> 
> @@ -2131,6 +2133,155 @@ update_range_test (struct range_entry *range,
> struct range_entry *otherrange,
>    return true;
>  }
> 
> +/* Optimize X == CST1 || X == CST2
> +   if popcount (CST1 ^ CST2) == 1 into
> +   (X & ~(CST1 ^ CST2)) == (CST1 & ~(CST1 ^ CST2)).
> +   Similarly for ranges.  E.g.
> +   X != 2 && X != 3 && X != 10 && X != 11
> +   will be transformed by the previous optimization into
> +   (X - 2U) <= 1U && (X - 10U) <= 1U
> +   and this loop can transform that into
> +   ((X & ~8) - 2U) <= 1U.  */
> +
> +static bool
> +try_transfer_range_tests_1 (enum tree_code opcode, int i, int j, tree
type,
> +			    tree lowi, tree lowj, tree highi, tree highj,
> +			    vec<operand_entry_t> *ops,
> +			    struct range_entry *ranges)
> 
> The function names are bad, you aren't transfering anything, but
optimizing.
> Please rename try_transfer_range_tests to optimize_range_tests_1 and
> try_transfer_range_tests_{1,2} to optimize_range_tests_{2,3} or perhaps
> better yet to optimize_range_tests_{xor,diff}.

try_transfer_range_tests is changed to optimize_range_tests_1
try_transfer_range_tests_1 is changed to optimize_range_tests_xor
try_transfer_range_tests_2 is changed to optimize_range_tests_diff

> Also, perhaps instead of passing ranges and i and j to these two functions
> you could pass struct range_entry *range1, struct range_entry *range2
> (caller would pass ranges + i, ranges + j).

Updated to rangei and rangej since we use i, j, lowi, lowj etc at other
places.
 
> +/* It does some common checks for function try_transfer_range_tests_1
> and
> +   try_transfer_range_tests_2.
> 
> Please adjust the comment for the renaming.  Please change trans_option to
> bool optimize_xor.

Updated.

> +	  if (trans_option == 1)
> +	    {
> +	      if (try_transfer_range_tests_1 (opcode, i, j, type, lowi,
lowj,
> +					      highi, highj, ops, ranges))
> +		{
> +		  any_changes = true;
> +		  break;
> +		}
> +	    }
> +	  else if (trans_option == 2)
> +	    {
> +	      if (try_transfer_range_tests_2 (opcode, i, j, type, lowi,
lowj,
> +					      highi, highj, ops, ranges))
> +		{
> +		  any_changes = true;
> +		  break;
> +		}
> +	    }
> 
> I'd prefer
> 	if (optimize_xor)
> 	  any_changes
> 	    = optimize_range_tests_xor (opcode, type, lowi, lowj, highi,
> 					highj, ops, ranges + i, ranges + j);
> 	else
> 	  any_changes
> 	    = optimize_range_tests_xor (opcode, type, lowi, lowj, highi,
> 					highj, ops, ranges + i, ranges + j);
> 	if (any_changes)
> 	  break;

This is incorrect. The "any_changes" should be "|=" of the return values.

In the updated patch, I changed the code segment as

	  bool changes;
	  ...
	  if (optimize_xor)
	    changes = optimize_range_tests_xor (opcode, type, lowi, lowj,
						highi, highj, ops,
						ranges + i, ranges + j);
	  else
	    changes = optimize_range_tests_diff (opcode, type, lowi, lowj,
						 highi, highj, ops,
						 ranges + i, ranges + j);
	  if (changes)
	    {
	      any_changes = true;
	      break;
	    }

Is it OK?

Thanks!
-Zhenqiang

> Ok with those changes.
> 
> 	Jakub

Attachment: reassoc-final.patch
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]