Re: [PATCH] Fix four bugs in combine

[Adam Nemet <anemet at caviumnetworks dot com>]

Paolo Bonzini writes:
> > ? ? ?if (!single_pred_p (this_basic_block)
> > ? ? ? ? ?|| single_pred (this_basic_block)->index != this_basic_block->index - 1)
> > ? ? ? ?label_tick_ebb_start = label_tick;
> >
> > Is this condition to start an EBB guaranteed to be valid by the cfglayout?
> 
> It is only a sufficient condition.
> 
> Since the blocks are visited in order of index, the data at the end of
> the previous block is still valid if the new block has the previous
> block as the predecessor.  I had missed the case of EXIT_BLOCK
> sitting between ENTRY_BLOCK and its successor.
> 
> For this particular bug, I'd rather specialize
> "|| single_pred (this_basic_block) == ENTRY_BLOCK"
> since we know the ENTRY_BLOCK is not processed by
> FOR_EACH_BB.  So we can keep the ticks monotonic.

Hm, what in my solution makes the ticks non-monotonic?  (I'm always going in
index order and start with 0 for the entry block.  I just add one to the index
to get the tick but that preserves monotonicity.)

Also above I guess you mean:

@@ -1121,7 +1126,8 @@ combine_instructions (rtx f, unsigned in
       mem_last_set = -1;
       label_tick = this_basic_block->index;
       if (!single_pred_p (this_basic_block)
-	  || single_pred (this_basic_block)->index != label_tick - 1)
+	  || (single_pred (this_basic_block)->index != label_tick - 1
+	      && single_pred (this_basic_block) != ENTRY_BLOCK_PTR))
 	label_tick_ebb_start = label_tick;
       rtl_profile_for_bb (this_basic_block);
       for (insn = BB_HEAD (this_basic_block);

I thought of that but that does not work if you have multiple successors to
the ENTRY_BLOCK (whatever that means).

I think my version captures the logic (you describe above) more closely: keep
using the state from previous iteration if the block there is our single
predecessor.

> For the other three, I agree with Adam's solutions.

Thanks.

Adam