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] Don't bypass blocks with multiple latch edges (PR middle-end/54838)


On Thu, Nov 29, 2012 at 6:43 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Yikes, sorry, it wasn't clear to me what PROP_loops really does.  Anyway,
>> I think I have a better fix now.  The problem is just that when removing
>> BB 4 (which was a header), we have to zap ->header and ->latch.  We
>> already have code for this:
>>
>>   if (current_loops != NULL
>>       && e->src->loop_father->latch == e->src)
>>     {
>>       /* ???  Now we are creating (or may create) a loop
>>          with multiple entries.  Simply mark it for
>>          removal.  Alternatively we could not do this
>>          threading.  */
>>       e->src->loop_father->header = NULL;
>>       e->src->loop_father->latch = NULL;
>>     }
>>
>> but the thing is that when there are multiple latch edges, then
>> ->latch is NULL.  So we need to keep track of how many latch edges
>> the header has.  Regtested/bootstrapped on x86_64, ok for trunk?
>>
>> Can I get rid of may_be_loop_header (and just use n_latch_edges > 0
>> instead at that one place) in a followup?
>>
>> 2012-11-29  Marek Polacek  <polacek@redhat.com>
>>
>>       PR middle-end/54838
>>       * cprop.c (bypass_block): Set header and latch to NULL when
>>       BB has more than one latch edge.
>>       (n_latches): New variable.
>
> This looks good on principle, but can't we do better now that we have the loop
> structure?   Can't we compute is_loop_header instead of may_be_loop_header and
> simplify the condition under which we mark the loop for removal?  Or maybe we
> should call disambiguate_loops_with_multiple_latches on entry of the pass?
>
> Richard, what would be the "modern" approach to solving the problem here?

RTL cprop seems to run both before and after RTL loop optimizers (currently
after RTL loop optimizers we throw away loops - an arbitrary chosen point
before IRA across which I could not get things to work).  Thus you could do

  if (current_loops)
    is_loop_header = bb == bb->loop_father->header;
  else
    {
  may_be_loop_header = false;
  FOR_EACH_EDGE (e, ei, bb->preds)
    if (e->flags & EDGE_DFS_BACK)
      {
        may_be_loop_header = true;
        break;
      }
    }

I don't understand

      /* The irreducible loops created by redirecting of edges entering the
         loop from outside would decrease effectiveness of some of the
         following optimizations, so prevent this.  */
      if (may_be_loop_header
          && !(e->flags & EDGE_DFS_BACK))
        {
          ei_next (&ei);
          continue;
        }

why isn't this simply

      if (may_be_loop_header)
        {
          ei_next (&ei);
          continue;
        }

?  It looks like the code tries to allow "rotating" a loop - but that's only
good if bb has exactly two predecessors (one entry and one latch edge).
And even then it requires to manually update the loop structures (update
what the new header and latch blocks are).

That said, removing the !(e->flags & EDGE_DFS_BACK) condition seems
to fix the ICE.  Threading across a loop header is in fact complicated
(see the special routine tree-ssa-threadupdate.c:thread_through_loop_header
necessary for that).  Let's declare the GIMPLE level did all interesting
threadings through headers.

Btw, if a pass wants to look at anything loop related _besides_ loop headers
it has to call loop_optimizer_init.  The only thing that is guaranteed
to be kept
up-to-date when PROP_loops is set (and thus current_loops != NULL) is
the header field in the loop tree.

Richard.

> --
> Eric Botcazou


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