ifcvt.c question.

ifcvt.c question.

[Ramana Radhakrishnan]

[-- Attachment #1: Type: text/plain, Size: 3909 bytes --]

Hi,

While experimenting to see if some of the bits in the ARM backend which
do predicated execution can be removed in favour of the more generic
bits in ifcvt.c. I attempted to turn on conditional calls using the
predicable attribute on the ARM port.


I've run into an ICE in the ifcvt pass for the following testcase. 

typedef int SItype __attribute__ ((mode (SI)));
typedef int DItype __attribute__ ((mode (DI)));

SItype
__mulvsi3 (SItype a, SItype b)
{
  const DItype w = (DItype) a * (DItype) b;

  if ((SItype) (w >> (4 * 8)) != (SItype) w >> ((4 * 8) - 1))
    abort ();

  return w;
}

test.c:14:1: internal compiler error: in merge_if_block, at ifcvt.c:2968
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

The ICE is because the following assert fails.

      /* We cannot merge the JOIN.  */
      /* The outgoing edge for the current COMBO block should already
be correct.  Verify this.  */
      gcc_assert (single_succ_p (combo_bb)
  && single_succ (combo_bb) == join_bb);



Here's what ifcvt does before the crash . 

IF-THEN block found, pass 1, start block 2 [insn 5], then 3 [14], join 1
[-1]
rescanning insn with uid = 15.
deleting insn with uid = 15.
1 insn was converted to conditional execution.
merging block 3 into block 2
deleting insn with uid = 13.
deleting insn with uid = 14.
changing bb of uid 15
  from 3 to 2


The insns deleted are : 

(jump_insn 13 12 14 2 test.c:9 (set (pc)
        (if_then_else (eq (reg:CC 24 cc)
                (const_int 0 [0x0]))
            (return)
            (pc))) 258 {*cond_return} (expr_list:REG_DEAD (reg:CC 24 cc)
        (expr_list:REG_BR_PROB (const_int 9996 [0x270c])
            (nil))))

(note 14 13 15 3 [bb 3] NOTE_INSN_BASIC_BLOCK)

insn 15 whose bb is changed from 3 to 2 is the call to abort. 


(call_insn 15 14 16 3 test.c:10 (parallel [
            (call (mem:SI (symbol_ref:SI ("abort") [flags 0x41]
<function_decl 0x2aaaaab7be00 abort>) [0 S4 A32])
                (const_int 0 [0x0]))
            (use (const_int 0 [0x0]))
            (clobber (reg:SI 14 lr))
        ]) 251 {*call_symbol} (expr_list:REG_NORETURN (const_int 0
[0x0])
        (expr_list:REG_EH_REGION (const_int 0 [0x0])
            (nil)))
    (nil))

In this case before if-conversion you have a basic block structure of
the following form. 

   [2] (cond_return is the last instruction)
    |
   [3] (call abort)  (REG_EH_NO_RETURN)
    |
   [1] - Exit block 


ifcvt then goes ahead and converts the call to abort into a conditional
call and should ideally convert the conditional return as a normal
return i.e. by moving the last instruction in [2] into [3] as an
unconditional return. It does the first part correctly which is
converting the call to a conditional call but incorrectly removes the
unconditional return. 

Note however that thread_prologue_and_epilogue inserts an extra return
instruction in basic block 4 but this gets removed by cfg_cleanup the
next time it is run before dse2 because bb4 is unreachable. The ARM
backend is correct in generating a conditional return in this case,
because it is capable of generating a load multiple instruction that
restores the PC as well as the saved integer registers. 

I wonder if the best way to fix this is to teach ifcvt.c to handle
conditional returns.  Should I be fixing this inside ifcvt.c by handling
conditional returns specially or should I be looking elsewhere ? 

I've attached the dumps and the original patch to turn on the predicable
attribute for the ARM backend with this email. 

Thanks in advance for the answers. 

cheers
Ramana


Attachments 

1. Patch to turn on predicable attributes on all the ARM call patterns. 
2. Dumps of the testcase that are relevant, test.c.*.peephole2,
test.c.*.dse2, test.c.*.ce3. (dumps.tar.bz2)
3. Testcase test.c 


-- 
Ramana Radhakrishnan
GNU Tools 
ARM Ltd.

[-- Attachment #2: calls_predicable.patch --]
[-- Type: text/x-patch, Size: 2353 bytes --]

Index: arm.md
===================================================================
--- arm.md	(revision 150565)
+++ arm.md	(working copy)
@@ -4860,7 +4860,8 @@ (define_insn "*arm_movdi"
   [(set_attr "length" "8,12,16,8,8")
    (set_attr "type" "*,*,*,load2,store2")
    (set_attr "pool_range" "*,*,*,1020,*")
-   (set_attr "neg_pool_range" "*,*,*,1008,*")]
+   (set_attr "neg_pool_range" "*,*,*,1008,*")
+   (set_attr "predicable" "yes")]
 )
 
 (define_split
@@ -8446,7 +8447,8 @@ (define_insn "*call_value_reg_armv5"
    (clobber (reg:SI LR_REGNUM))]
   "TARGET_ARM && arm_arch5"
   "blx%?\\t%1"
-  [(set_attr "type" "call")]
+  [(set_attr "type" "call")
+   (set_attr "predicable" "yes")]
 )
 
 (define_insn "*call_value_reg_arm"
@@ -8460,7 +8462,8 @@ (define_insn "*call_value_reg_arm"
   return output_call (&operands[1]);
   "
   [(set_attr "length" "12")
-   (set_attr "type" "call")]
+   (set_attr "type" "call")
+   (set_attr "predicable" "yes")]
 )
 
 (define_insn "*call_value_mem"
@@ -8474,7 +8477,8 @@ (define_insn "*call_value_mem"
   return output_call_mem (&operands[1]);
   "
   [(set_attr "length" "12")
-   (set_attr "type" "call")]
+   (set_attr "type" "call")
+   (set_attr "predicable" "yes")]
 )
 
 (define_insn "*call_value_reg_thumb1_v5"
@@ -8525,7 +8529,8 @@ (define_insn "*call_symbol"
   {
     return NEED_PLT_RELOC ? \"bl%?\\t%a0(PLT)\" : \"bl%?\\t%a0\";
   }"
-  [(set_attr "type" "call")]
+  [(set_attr "type" "call")
+   (set_attr "predicable" "yes")]
 )
 
 (define_insn "*call_value_symbol"
@@ -8541,7 +8546,8 @@ (define_insn "*call_value_symbol"
   {
     return NEED_PLT_RELOC ? \"bl%?\\t%a1(PLT)\" : \"bl%?\\t%a1\";
   }"
-  [(set_attr "type" "call")]
+  [(set_attr "type" "call")
+   (set_attr "predicable" "yes")]
 )
 
 (define_insn "*call_insn"
@@ -8608,7 +8614,8 @@ (define_insn "*sibcall_insn"
   "*
   return NEED_PLT_RELOC ? \"b%?\\t%a0(PLT)\" : \"b%?\\t%a0\";
   "
-  [(set_attr "type" "call")]
+  [(set_attr "type" "call")
+   (set_attr "predicable" "yes")]
 )
 
 (define_insn "*sibcall_value_insn"
@@ -8621,7 +8628,8 @@ (define_insn "*sibcall_value_insn"
   "*
   return NEED_PLT_RELOC ? \"b%?\\t%a1(PLT)\" : \"b%?\\t%a1\";
   "
-  [(set_attr "type" "call")]
+  [(set_attr "type" "call")
+   (set_attr "predicable" "yes")]
 )
 
 ;; Often the return insn will be the same as loading from memory, so set attr

[-- Attachment #3: dumps.tar.bz2 --]
[-- Type: application/x-bzip-compressed-tar, Size: 4694 bytes --]

[-- Attachment #4: test.c --]
[-- Type: text/x-csrc, Size: 292 bytes --]


typedef int SItype __attribute__ ((mode (SI)));
typedef int DItype __attribute__ ((mode (DI)));
void abort (void);
SItype
__mulvsi3 (SItype a, SItype b)
{
  const DItype w = (DItype) a * (DItype) b;

  if ((SItype) (w >> (4 * 8)) != (SItype) w >> ((4 * 8) - 1))
    abort ();

  return w;
}

Re: ifcvt.c question.

[Steven Bosscher]

On Mon, Aug 10, 2009 at 1:16 PM, Ramana
Radhakrishnan<ramana.radhakrishnan@arm.com> wrote:
> I wonder if the best way to fix this is to teach ifcvt.c to handle
> conditional returns.

Yes. This is a bug in the middle-end.  I can only guess why you are
the first to run into this (the ia64 return instruction is not
predicable?) but clearly the assert is bogus if the conditional branch
of the IF-block is actually a conditional return.

> Should I be fixing this inside ifcvt.c by handling
> conditional returns specially

I think you should fix this inside ifcvt.c, but try to make the
handing of cond_return not too specially ;-)

Alternatively, you could add a conditional call pattern and a
post-reload splitter to the ARM backend, and disallow the
EXIT_BLOCK_PTR as the JOIN-block in ifcvt.c.

Ciao!
Steven

Re: ifcvt.c question.

[Ramana Radhakrishnan]

On Mon, 2009-08-10 at 15:09 +0200, Steven Bosscher wrote:
> On Mon, Aug 10, 2009 at 1:16 PM, Ramana
> Radhakrishnan<ramana.radhakrishnan@arm.com> wrote:
> > I wonder if the best way to fix this is to teach ifcvt.c to handle
> > conditional returns.
> 
> Yes. This is a bug in the middle-end.  I can only guess why you are
> the first to run into this (the ia64 return instruction is not
> predicable?) but clearly the assert is bogus if the conditional branch
> of the IF-block is actually a conditional return.

I did try to see what happened with the ia64 port - The ia64 return insn
is predicable but th port doesn't like to conditionalize the return for
this particular case because current_frame_info.r[reg_save_b0] is 32 and
not 0 as required. Not knowing enough about the ia64 backend I gave up
at this point assuming that the port was DTRT.

> 
> > Should I be fixing this inside ifcvt.c by handling
> > conditional returns specially
> 
> I think you should fix this inside ifcvt.c, but try to make the
> handing of cond_return not too specially ;-)
> 
> Alternatively, you could add a conditional call pattern and a
> post-reload splitter to the ARM backend, and disallow the
> EXIT_BLOCK_PTR as the JOIN-block in ifcvt.c.

I have added a conditional call pattern by saying that the call pattern
is predicable - I don't see what else you mean here. What do you want to
split here ? The conditional return + call into a cond_call + return or
conditional return into a conditional branch and return ... ( both of
which are impossible really ! )

I don't see how one can fix this without special casing for a return
here. Either fix it inside ifcvt.c as a special case or look for a way
of doing this from the hooks that ifcvt appears to provide . 

Thanks,
Ramana 




> 
> Ciao!
> Steven

Re: ifcvt.c question

[Roger Sayle]

On Sun, 29 May 2005, Steven Bosscher wrote:
> I don't understand what lines 2728 to 2741 are for.  Could someone give
> an example of when we can have a then_bb that has no successors, but
> still ends in something that satisfies simplejump_p()?  What kind of
> strange indirect jump would that be?  And shouldn't the check just use
> computed_jump_p() if that is what it is looking for??

For the benefit of the list, I did some archeology to determine when
this functionality was written and how it evolved to be so broken now.

The mistake was a bug introduced by Michael Meisner back in 2000.
The functionality was originally introduced by Richard Henderson to
allow predication of then blocks that end in noreturn calls.

2000-05-31  Richard Henderson  <rth@cygnus.com>

        * ifcvt.c (merge_if_block): Be prepared for JOIN to have no
        remaining edges.
        (find_if_block): Allow THEN with no outgoing edges.
        * flow.c (merge_blocks_nomove): Remove a barrier not following
        a jump as well.

posted: http://gcc.gnu.org/ml/gcc-patches/2000-05/msg01711.html


Unfortunately, this exposed a bug in the d30v port that caused a
regression in gcc.c-torture/execute/920428-1.c.  As I suggested on
IRC, this was a then-block that ended in a non-local goto, or more
precisely an indirect jump.

Michael tried to resolve this problem by excluding those blocks that
ended in indirect jumps.  His mistake was to assume that the way to
eliminate indirect jumps was to limit this code to simple jumps!
Doh!  This inadvertantly disabled RTH's optimization, and the
optimizations' code has been dead ever since.

2000-08-19  Michael Meissner  <meissner@redhat.com>

        * ifcvt.c (find_if_block): Do not assume that a THEN block has any
        instructions in it before checking for indirect jumps.

        * ifcvt.c (find_if_block): Do not consider a THEN block that ends
        in an indirect jump as a potential for conditional execution.

        * d30v.h (d30v_init_expanders): Don't declare here.
        * d30v-protos.h (d30v_init_expanders): Declare here with a valid
        prototype.

posted: http://gcc.gnu.org/ml/gcc-patches/2000-08/msg00736.html


RTH should probably have added a testcase that checked whether this
code was being optimized, so we'd know when it regressed.  I also can't
immediately see why a block that ends in an indirect jump shouldn't
be considered for conditional execution, and he might well have been
papering over another problem in the compiler (esp. the D30V backend).

Of course now that support for D30V has been removed from CVS, it's
difficult to diagnose whether this really was just a backend problem,
and whether we should just return the code to a form similar to the
one RTH originally contributed.

I hope this answers your question.

Roger
--

ifcvt.c question

[Steven Bosscher]

Hi,

ifcvt.c has this code in find_if_block():

  2718    /* If the THEN block has no successors, conditional execution can still
  2719       make a conditional call.  Don't do this unless the ELSE block has
  2720       only one incoming edge -- the CFG manipulation is too ugly otherwise.
  2721       Check for the last insn of the THEN block being an indirect jump, which
  2722       is listed as not having any successors, but confuses the rest of the CE
  2723       code processing.  ??? we should fix this in the future.  */
  2724    if (EDGE_COUNT (then_bb->succs) == 0)
  2725      {
  2726        if (single_pred_p (else_bb))
  2727          {
  2728            rtx last_insn = BB_END (then_bb);
  2729
  2730            while (last_insn
  2731                   && NOTE_P (last_insn)
  2732                   && last_insn != BB_HEAD (then_bb))
  2733              last_insn = PREV_INSN (last_insn);
  2734
  2735            if (last_insn
  2736                && JUMP_P (last_insn)
  2737                && ! simplejump_p (last_insn))
  2738              return FALSE;
  2739
  2740            join_bb = else_bb;
  2741            else_bb = NULL_BLOCK;
  2742          }
  2743        else
  2744          return FALSE;
  2745      }
  2746

I don't understand what lines 2728 to 2741 are for.  Could someone give
an example of when we can have a then_bb that has no successors, but
still ends in something that satisfies simplejump_p()?  What kind of
strange indirect jump would that be?  And shouldn't the check just use
computed_jump_p() if that is what it is looking for??

Gr.
Steven