Re: [PATCH GCC]Cleanup interface of iv_ca_add_use and the calls to it

[Bin.Cheng]

On Wed, Aug 27, 2014 at 4:23 PM, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Wed, Aug 27, 2014 at 10:10 AM, Bin Cheng <bin.ch...@arm.com> wrote:
>> Hi,
>> As I analyzed in bug pr62178, current candidate selecting algorithm can't
>> find out the optimal solution in some scenarios.  I am trying to improve it
>> but before that, I need to clean up the interface of iv_ca_add_use and the
>> calls to it.  The two calls to the function are controlled by a boolean
>> argument, and the second call is always fired if the first one doesn't give
>> any result.  This patch encapsulates logic of the two calls into function
>> iv_ca_add_use and cleanups the interface.
>> Another change is remove the check in below code.
>>
>>   gcc_assert (ivs->upto >= use->id);
>>
>>   if (ivs->upto == use->id)
>>     {
>>       ivs->upto++;
>>       ivs->bad_uses++;
>>     }
>>
>> It can be proved in an inductive approach that ivs->up_to always equals to
>> use->id at the position.
>>
>> This patch does not change code logic at all, anyway, it passes bootstrap
>> and regtest on x86_64/x86.  So is it OK?
>
> Ok (I suppose you checked that it really generates the same code on
> a set of files?)

Yes, I compared object files in stage3 gcc directory, only
miscellaneous checksum files and tree-ssa-loop-ivopts.o itself are
different.  I suppose checksum files are trivial, right?

Here attached the right patch, the old one missed one local variable
declaration.

I will commit in 24 hours if there is no other objection.

Thanks,
bin

Index: gcc/tree-ssa-loop-ivopts.c
===================================================================
--- gcc/tree-ssa-loop-ivopts.c  (revision 214413)
+++ gcc/tree-ssa-loop-ivopts.c  (working copy)
@@ -5415,36 +5415,40 @@ iv_ca_set_cp (struct ivopts_data *data, struct iv_
 }
 
 /* Extend set IVS by expressing USE by some of the candidates in it
-   if possible. All important candidates will be considered
-   if IMPORTANT_CANDIDATES is true.  */
+   if possible.  Consider all important candidates if candidates in
+   set IVS don't give any result.  */
 
 static void
 iv_ca_add_use (struct ivopts_data *data, struct iv_ca *ivs,
-              struct iv_use *use, bool important_candidates)
+              struct iv_use *use)
 {
   struct cost_pair *best_cp = NULL, *cp;
   bitmap_iterator bi;
-  bitmap cands;
   unsigned i;
+  struct iv_cand *cand;
 
   gcc_assert (ivs->upto >= use->id);
+  ivs->upto++;
+  ivs->bad_uses++;
 
-  if (ivs->upto == use->id)
+  EXECUTE_IF_SET_IN_BITMAP (ivs->cands, 0, i, bi)
     {
-      ivs->upto++;
-      ivs->bad_uses++;
-    }
-
-  cands = (important_candidates ? data->important_candidates : ivs->cands);
-  EXECUTE_IF_SET_IN_BITMAP (cands, 0, i, bi)
-    {
-      struct iv_cand *cand = iv_cand (data, i);
-
+      cand = iv_cand (data, i);
       cp = get_use_iv_cost (data, use, cand);
-
       if (cheaper_cost_pair (cp, best_cp))
        best_cp = cp;
     }
+   
+  if (best_cp == NULL)
+    {
+      EXECUTE_IF_SET_IN_BITMAP (data->important_candidates, 0, i, bi)
+       {
+         cand = iv_cand (data, i);
+         cp = get_use_iv_cost (data, use, cand);
+         if (cheaper_cost_pair (cp, best_cp))
+           best_cp = cp;
+       }
+    }
 
   iv_ca_set_cp (data, ivs, use, best_cp);
 }
@@ -5878,18 +5882,9 @@ try_add_cand_for (struct ivopts_data *data, struct
   struct iv_ca_delta *best_delta = NULL, *act_delta;
   struct cost_pair *cp;
 
-  iv_ca_add_use (data, ivs, use, false);
+  iv_ca_add_use (data, ivs, use);
   best_cost = iv_ca_cost (ivs);
-
   cp = iv_ca_cand_for_use (ivs, use);
-  if (!cp)
-    {
-      ivs->upto--;
-      ivs->bad_uses--;
-      iv_ca_add_use (data, ivs, use, true);
-      best_cost = iv_ca_cost (ivs);
-      cp = iv_ca_cand_for_use (ivs, use);
-    }
   if (cp)
     {
       best_delta = iv_ca_delta_add (use, NULL, cp, NULL);