diff mbox

[RFA] Fix an ia64 Ada bootstrap problem

Message ID 5285CB27.50503@redhat.com
State New
Headers show

Commit Message

Jeff Law Nov. 15, 2013, 7:20 a.m. UTC
I'm still unable to bootstrap the ia64 port if I back out Kirill's most 
recent patch.

The erroneous-path-isolation code has exposed a latent bug in the RTL 
if-conversion pass which runs after combine (ie no longer in cfglayout 
mode).

We have this as we leave combine:





(insn 107 106 108 10 (set (reg:BI 443)
         (eq:BI (reg/v/f:DI 399 [ gnu_expr ])
             (const_int 0 [0]))) 
../../gcc/gcc/ada/gcc-interface/decl.c:6238 311 {*cmpdi_normal}
      (nil))
(jump_insn 108 107 109 10 (set (pc)
         (if_then_else (ne (reg:BI 443)
                 (const_int 0 [0]))
             (label_ref 424)
             (pc))) ../../gcc/gcc/ada/gcc-interface/decl.c:6238 318 
{*br_true}
      (expr_list:REG_DEAD (reg:BI 443)
         (int_list:REG_BR_PROB 5359 (nil)))
  -> 424)

[ lots of insns ]

(code_label 424 414 425 52 1553 "" [1 uses])
(note 425 424 427 52 [bb 52] NOTE_INSN_BASIC_BLOCK)
(insn 427 425 430 52 (set (reg:HI 581 [ MEM[(union tree_node 
*)0B].base.code ])
         (mem/v/j:HI (reg/v/f:DI 399 [ gnu_expr ]) [0 MEM[(union 
tree_node *)0B].base.code+0 S2 A128])) 
../../gcc/gcc/ada/gcc-interface/decl.c:6246 4 {movhi_internal}
      (expr_list:REG_DEAD (reg/v/f:DI 399 [ gnu_expr ])
         (expr_list:REG_UNUSED (reg:HI 581 [ MEM[(union tree_node 
*)0B].base.code ])
             (expr_list:REG_EQUAL (mem/v/j:HI (const_int 0 [0]) [0 
MEM[(union tree_node *)0B].base.code+0 S2 A128])
                 (nil)))))
(insn 430 427 476 52 (trap_if (const_int 1 [0x1])
         (const_int 0 [0])) 363 {*trap}
      (nil))


Something deletes/moves insn 427 out of the way (most likely predicated 
and shoved into bb10).

After that point bb52 just contains the trap.




We get into ifcvt.c::find_cond_trap.

We determine that trap_bb is else_bb, insert the conditional trap into 
test_bb and cleanup/remove the trap block.  All that's left to do is 
cleanup test_bb.  Remember that the THEN edge is canonically the 
fallthru edge so in this case when the branch was taken it reached the trap.

That cleanup code looks like:


  /* Wire together the blocks again.  */
   if (current_ir_type () == IR_RTL_CFGLAYOUT)
     single_succ_edge (test_bb)->flags |= EDGE_FALLTHRU;
   else
     {
       rtx lab, newjump;

       lab = JUMP_LABEL (jump);
       newjump = emit_jump_insn_after (gen_jump (lab), jump);
       LABEL_NUSES (lab) += 1;
       JUMP_LABEL (newjump) = lab;
       emit_barrier_after (newjump);
     }
   delete_insn (jump);


We're running after combine and thus we're not in cfglayout mode.

The code creates a new unconditional jump to the label referenced in the 
original conditional jump.  We then emit the new unconditional jump into 
the IL and delete the conditional jump.  That is fine if the trap was in 
the then (fallthru) block.

But that makes *no* sense when the trap is in the else block.  The label 
has been deleted from the insn chain and more importantly, we want to 
fallthru if we do not trap!

Thankfully the the CFG checking code detected this inconsistency.  It's 
been latent since 2002!  Clearly we aren't doing a lot of optimizing 
conditional jumps over/to traps into conditional traps!

Anyway, the fix is trivial.  When trap_bb == then_bb, run the code as 
is.  When trap_bb == else_bb we only want to remove the conditinoal jump 
as we want to fallthru if the conditional trap doesn't trigger.

With this patch applied and Kirill's patch removed, I can almost 
bootstrap the ia64 port with Ada enabled (comparison failure that AFAICT 
is not related to the isolate-erroneous-paths optimization)



OK for the trunk if it passes a bootstrap & regtest on x86_64 overnight?

Jeff

ps.  An Itanic with 108 processors is still, well, an Itanic.
* ifcvt.c (find_cond_trap): Properly handle case where
	trap_bb == else_bb.

Comments

Eric Botcazou Nov. 15, 2013, 4:42 p.m. UTC | #1
> But that makes *no* sense when the trap is in the else block.  The label
> has been deleted from the insn chain and more importantly, we want to
> fallthru if we do not trap!
> 
> Thankfully the the CFG checking code detected this inconsistency.  It's
> been latent since 2002!  Clearly we aren't doing a lot of optimizing
> conditional jumps over/to traps into conditional traps!

Not clear IMO, this seems to have been broken by r120686.

> Anyway, the fix is trivial.  When trap_bb == then_bb, run the code as
> is.  When trap_bb == else_bb we only want to remove the conditinoal jump
> as we want to fallthru if the conditional trap doesn't trigger.
> 
> With this patch applied and Kirill's patch removed, I can almost
> bootstrap the ia64 port with Ada enabled (comparison failure that AFAICT
> is not related to the isolate-erroneous-paths optimization)
> 
> 
> 
> OK for the trunk if it passes a bootstrap & regtest on x86_64 overnight?

Yes, this looks fine to me.
diff mbox

Patch

diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index fafff9d..17d26c5 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -3694,7 +3694,7 @@  find_cond_trap (basic_block test_bb, edge then_edge, edge else_edge)
   /* Wire together the blocks again.  */
   if (current_ir_type () == IR_RTL_CFGLAYOUT)
     single_succ_edge (test_bb)->flags |= EDGE_FALLTHRU;
-  else
+  else if (trap_bb == then_bb)
     {
       rtx lab, newjump;