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

[Zhenqiang Chen]

> -----Original Message-----
> From: Jakub Jelinek [mailto:ja...@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

reassoc-final.patch
Description: Binary data