> -----Original Message----- > From: Jakub Jelinek [mailto:[email protected]] > Sent: Monday, October 14, 2013 4:49 PM > To: Zhenqiang Chen > Cc: [email protected] > 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
reassoc-final.patch
Description: Binary data
