Bug 24762 - [killloop-branch] code motion of non-invariant expressions with hard registers.
Summary: [killloop-branch] code motion of non-invariant expressions with hard registers.
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: unknown
: P3 normal
Target Milestone: ---
Assignee: Zdenek Dvorak
URL:
Keywords: EH, wrong-code
Depends on:
Blocks: 22366 24408
  Show dependency treegraph
 
Reported: 2005-11-09 20:11 UTC by Steven Bosscher
Modified: 2006-02-09 22:34 UTC (History)
2 users (show)

See Also:
Host:
Target: ia64
Build:
Known to work:
Known to fail:
Last reconfirmed: 2005-11-11 17:52:46


Attachments
loop2_init dump (2.11 KB, text/plain)
2005-11-09 20:12 UTC, Steven Bosscher
Details
loop2_invariant dump (1.88 KB, text/plain)
2005-11-09 20:12 UTC, Steven Bosscher
Details
Fix df-scan.c (425 bytes, patch)
2006-01-14 17:24 UTC, Steven Bosscher
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steven Bosscher 2005-11-09 20:11:26 UTC
The killloop-branch produces wrong code for the following test due to a bug in loop-invariant.c or code related to it.

extern "C" void abort(void);
class runtime_error {};
void test01(int iters)
{
    for (int i = 0;   i < iters;   ++i)
    {
        try  {
            throw runtime_error();
            abort();
        } catch (runtime_error&) {  }
    }
}

int main(int argc, char* argv[])
{
    test01(1);
    return 0;
}

The problem is that the following expression is moved out of the loop:

(insn 64 63 54 3 (set (reg:DI 350)
        (reg:DI 16 r16)) 5 {*movdi_internal} (nil)
    (nil))

See the attached dumps.  So far I can only reproduce the bug on IA-64.  Obviously, exceptions are involved somehow.  On ia64, there are also over 300 libjava failures that disappear when loop-invariant.c is disabled.
Comment 1 Steven Bosscher 2005-11-09 20:12:22 UTC
Created attachment 10197 [details]
loop2_init dump
Comment 2 Steven Bosscher 2005-11-09 20:12:43 UTC
Created attachment 10198 [details]
loop2_invariant dump
Comment 3 Andrew Pinski 2005-11-09 21:08:49 UTC
This is not an ia64 specific issue as far as I can see, on x86_64, we get:

(note 64 61 62 2 [bb 2] NOTE_INSN_BASIC_BLOCK)

(insn 62 64 63 2 (set (reg:DI 63)
        (reg:DI 0 ax)) -1 (nil)
    (nil))

(insn 63 62 53 2 (set (reg:DI 65)
        (reg:DI 1 dx)) -1 (nil)
    (nil))

so those could also be moved above the loop also (if you change the cost).
Comment 4 Andrew Pinski 2005-11-09 21:14:11 UTC
The sets come from dw2_build_landing_pads:
      emit_move_insn (cfun->eh->exc_ptr,
                      gen_rtx_REG (ptr_mode, EH_RETURN_DATA_REGNO (0)));
      emit_move_insn (cfun->eh->filter,
                      gen_rtx_REG (targetm.eh_return_filter_mode (),
                                   EH_RETURN_DATA_REGNO (1)));
Comment 5 Zdenek Dvorak 2005-11-09 22:30:16 UTC
I am testing the following patch:

Index: except.c
===================================================================
*** except.c    (revision 106702)
--- except.c    (working copy)
*************** dw2_build_landing_pads (void)
*** 1527,1543 ****

        /* If the eh_return data registers are call-saved, then we
         won't have considered them clobbered from the call that
!        threw.  Kill them now.  */
        for (j = 0; ; ++j)
        {
          unsigned r = EH_RETURN_DATA_REGNO (j);
          if (r == INVALID_REGNUM)
            break;
!         if (! call_used_regs[r])
!           {
!             emit_insn (gen_rtx_CLOBBER (VOIDmode, gen_rtx_REG (Pmode, r)));
!             clobbers_hard_regs = true;
!           }
        }

        if (clobbers_hard_regs)
--- 1527,1544 ----

        /* If the eh_return data registers are call-saved, then we
         won't have considered them clobbered from the call that
!        threw.  But we need to clobber all hard registers, since
!        df.c assumes that call clobbers are not definitions
!        (normally it is invalid to use call-clobbered register
!        after the call).  */
        for (j = 0; ; ++j)
        {
          unsigned r = EH_RETURN_DATA_REGNO (j);
          if (r == INVALID_REGNUM)
            break;
!
!         emit_insn (gen_rtx_CLOBBER (VOIDmode, gen_rtx_REG (Pmode, r)));
!         clobbers_hard_regs = true;
        }

        if (clobbers_hard_regs)
Comment 6 Daniel Berlin 2005-11-09 22:53:50 UTC
1. Call clobbers aren't definitions. They are clobbers (IE kills).  They do not generate a new value, they simply specify the old value is dead.  Thus, you don't mean to say "df.c assumes call clobbers ...", because it's not an assumption.


2. The patch is actually an incorrect workaround.  The real problem is that df.c on mainline doesn't get reaching definitions right for a number of reasons.

For example, explicit clobbers in CALL_INSN_FUNCTION_USAGE but *not* in regs_invalidated_by_call will be treated as gen's, because they don't fall into the special case in df_bb_rd_local_compute.  But they are not gen's.

Have you tried making sure CLOBBER's don't get into the rd_gen set?

I imagine that will fix your bug.
Comment 7 Daniel Berlin 2005-11-09 22:57:21 UTC
There are also a ton of other bugs related to hard regs in reaching definitions on df.c in mainline, i'm just guessing.

Try the df.c from the dataflow-branch, and see if it fixes the bug, because honestly, i think you are barking up the wrong tree here.

Clobbers are most *certainly* not new definitions.
Comment 8 Zdenek Dvorak 2005-11-09 23:15:53 UTC
Subject: Re:  [killloop-branch] code motion of non-invariant expressions with hard registers.

> ------- Comment #6 from dberlin at gcc dot gnu dot org  2005-11-09 22:53 -------
> 1. Call clobbers aren't definitions. They are clobbers (IE kills).  They do not
> generate a new value, they simply specify the old value is dead.  Thus, you
> don't mean to say "df.c assumes call clobbers ...", because it's not an
> assumption.
> 
> 
> 2. The patch is actually an incorrect workaround.  The real problem is that
> df.c on mainline doesn't get reaching definitions right for a number of
> reasons.
> 
> For example, explicit clobbers in CALL_INSN_FUNCTION_USAGE but *not* in
> regs_invalidated_by_call will be treated as gen's, because they don't fall into
> the special case in df_bb_rd_local_compute.  But they are not gen's.
> 
> Have you tried making sure CLOBBER's don't get into the rd_gen set?
> 
> I imagine that will fix your bug.

I am fairly sure you are wrong, in this case.  The code is like

bb1:
call (throw);

bb2:

reg = r15

where r15 is set by exception handling.  The problem is that there
currently simply is no insn that defines r15.
Comment 9 Daniel Berlin 2005-11-09 23:30:55 UTC
Okay, well, there are also bugs in df.c on mainline in regards to not creating uses for "always-live" registers and registers that are live over eh edges, for example.  (It does none of this).  Seriously. Hard regs are very badly broken in mainline df.c in terms of liveness and use-def/def-use chains.

Steven tells me flow.c gets liveness for your particular example wrong too though.  In the dataflow branch df.c, we would add a use for the "live on entry to eh" regs, in the artifical uses code for eh blocks, and it would just work.



Comment 10 Steven Bosscher 2005-11-09 23:45:05 UTC
Actually, flow.c does get it right.
From t.C.26.life1 (at -O1 -fno-move-loop-invariants):

;; Start of basic block 3, registers live: 0 [ap] 1 [r1] 12 [r12] 15 [r15] 16 [r16] 328 [sfp]
341 344
(code_label/s 62 30 65 3 8 "" [1 uses])

(note 65 62 63 3 [bb 3] NOTE_INSN_BASIC_BLOCK)

(insn 63 65 64 3 (set (reg:DI 348)
        (reg:DI 15 r15)) 5 {*movdi_internal} (nil)
    (expr_list:REG_DEAD (reg:DI 15 r15)
        (nil)))

(insn 64 63 54 3 (set (reg:DI 350)
        (reg:DI 16 r16)) 5 {*movdi_internal} (nil)
    (expr_list:REG_DEAD (reg:DI 16 r16)
        (nil)))

Comment 11 Daniel Berlin 2005-11-09 23:56:28 UTC
Subject: Re:  [killloop-branch] code motion of
	non-invariant expressions with hard registers.

On Wed, 2005-11-09 at 23:45 +0000, steven at gcc dot gnu dot org wrote:
> 
> ------- Comment #10 from steven at gcc dot gnu dot org  2005-11-09 23:45 -------
> Actually, flow.c does get it right.

Okay, then df.c on dataflow branch should get it right too.


Comment 12 Andrew Pinski 2005-11-11 17:52:46 UTC
Confirmed.
Comment 13 Steven Bosscher 2005-11-14 22:47:24 UTC
The dataflow-branch is also failing for this test case.  There is nothing in the df object that makes loop-invariant.c think that the insns setting those EH_RETURN_DATA_REGNO regs are not loop invariant.
Comment 14 Daniel Berlin 2005-11-14 23:10:23 UTC
Send me a dump of df_analyze (df, -1, DF_LR | DF_HARD_REGS | DF_ARTIFICIAL_USES), and point out what you say is wrong and i'll make it right.
Comment 15 Zdenek Dvorak 2005-11-15 14:27:52 UTC
Subject: Bug 24762

Author: rakdver
Date: Tue Nov 15 14:27:48 2005
New Revision: 107017

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=107017
Log:
	PR rtl-optimization/24762
	* except.c (dw2_build_landing_pads): Always emit clobbers for
	eh registers.


Modified:
    branches/killloop-branch/gcc/ChangeLog.killloop
    branches/killloop-branch/gcc/except.c

Comment 16 Steven Bosscher 2006-01-14 16:59:53 UTC
This is still not fixed on mainline, even with the new df implementation.  The code to record them is not executed (at least, not on AMD64).  The function responsible for recording them is df_bb_refs_record, specifically this code:

ifdef EH_RETURN_DATA_REGNO
  if ((df->flags & DF_HARD_REGS)
      && df_has_eh_preds (bb))
    {
      unsigned int i;
      /* Mark the registers that will contain data for the handler.  */
      if (current_function_calls_eh_return)
        for (i = 0; ; ++i)
          {
            unsigned regno = EH_RETURN_DATA_REGNO (i);
            if (regno == INVALID_REGNUM)
              break;
            df_ref_record (dflow, regno_reg_rtx[i], &regno_reg_rtx[i], bb, NULL,
                           DF_REF_REG_DEF, DF_REF_ARTIFICIAL | DF_REF_AT_TOP, false);
          }
    }
#endif

But current_function_calls_eh_return is false for the test case, even though the EH_RETURN_DATA_REGNO regs are set:

Breakpoint 3, df_bb_refs_record (dflow=0x11203f0, bb=0x2aaaaafaa780) at df-scan.c:1519
1519      if ((df->flags & DF_HARD_REGS)
(gdb) p debug_bb(bb)
;; basic block 5, loop depth 1, count 0
;; prev block 4, next block 6
;; pred:       4 (ab,abcall,eh)
;; succ:       7 [29.0%]  6 [71.0%]  (fallthru,loop_exit)
;; Registers live at start:  (nil)
(code_label/s 61 28 64 5 8 "" [1 uses])
(note 64 61 62 5 [bb 5] NOTE_INSN_BASIC_BLOCK)
(insn 62 64 63 5 (set (reg:DI 65)
        (reg:DI 0 ax)) 81 {*movdi_1_rex64} (nil)
    (nil))
(insn 63 62 53 5 (set (reg:DI 66)
        (reg:DI 1 dx)) 81 {*movdi_1_rex64} (nil)
    (nil))
(insn 53 63 54 5 (set (reg:CCZ 17 flags)
        (compare:CCZ (reg:DI 66)
            (const_int 1 [0x1]))) 2 {cmpdi_1_insn_rex64} (nil)
    (nil))
(jump_insn 54 53 65 5 (set (pc)
        (if_then_else (eq (reg:CCZ 17 flags)
                (const_int 0 [0x0]))
            (label_ref 30)
            (pc))) 511 {*jcc_1} (nil)
    (expr_list:REG_BR_PROB (const_int 2900 [0xb54])
        (nil)))
;; Registers live at end:  (nil)
$5 = void
(gdb) next
1524          if (current_function_calls_eh_return)
(gdb) p cfun->calls_eh_return
$6 = 0

As a result, we still consider insns 62 and 63 to be loop invariant:

;; Function void test01(int) (_Z6test01i)

Set in insn 22 is invariant (0), cost 0, depends on
Set in insn 25 is invariant (1), cost 0, depends on
Set in insn 26 is invariant (2), cost 0, depends on
Set in insn 62 is invariant (3), cost 4, depends on
Set in insn 63 is invariant (4), cost 4, depends on
Set in insn 33 is invariant (5), cost 0, depends on 3
Comment 17 Steven Bosscher 2006-01-14 17:24:30 UTC
Created attachment 10642 [details]
Fix df-scan.c

There doesn't seem to be a good reason to make adding the artificial defs for the EH_RETURN_DATA_REGNO regs conditional on current_function_calls_eh_return.
Comment 18 zadeck@gcc.gnu.org 2006-01-27 22:23:43 UTC
Subject: Bug 24762

Author: zadeck
Date: Fri Jan 27 22:23:32 2006
New Revision: 110312

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=110312
Log:
2006-01-27  Daniel Berlin  <dberlin@dberlin.org>
	    Kenneth Zadeck <zadeck@naturalbridge.com>

	PR rtl-optimization/24762 
	* doc/tm.texi: Added TARGET_EXTRA_LIVE_ON_ENTRY.
	* targhooks.c (hook_void_bitmap): New hook prototype.
	* targhoohs.h (hook_void_bitmap): Ditto.
	* bitmap.h (bitmap_head_def): Moved to coretypes.h.
	* coretypes.h (bitmap_head_def): Moved from bitmap.h.
	* target.h (live_on_entry): New function pointer.
	* df-scan.c (df_all_hard_regs): Removed.
	(df_scan_dump, df_hard_reg_init): Removed df_all_hard_regs.
	(df_scan_free_internal): Added df->entry_block_defs.
	(df_scan_alloc): Ditto.
	(df_scan_dump): Ditto.
	(df_uses_record): Plumbed flag field properly thru calls.
	Record EH_RETURN_DATA_REGNO in eh blocks unconditionally.
	This part fixes PR24762.
	(df_bb_refs_record): Added code to make the frame and arg
	pointers live in EH blocks.
	(df_refs_record): Added call to df_record_entry_block_defs.
	(df_record_entry_block_defs): New function.
	* df-core.c: Added comments to describe new artifical defs.
	* df.h (DF_REF_DIES_AFTER_THIS_USE): New flag in enum df_ref_flags.
	(entry_block_defs): New field in struct df.
	(df_all_hard_regs): Deleted.
	* target-def.h: Added TARGET_EXTRA_LIVE_ON_ENTRY.
	* df-problems.c (df_ru_bb_local_compute_process_def):
	Added code to handle artifical defs in the entry to a function.
	(df_ru_bb_local_compute): Ditto.
	(df_rd_bb_local_compute_process_def):  Ditto.
	(df_rd_bb_local_compute): Ditto.
	(df_lr_bb_local_compute): Ditto.
	(df_ur_bb_local_compute): Ditto.
	(df_urec_bb_local_compute):  Ditto.
	(df_chain_create_bb):  Ditto.
	(df_ur_local_finalize):	Removed entry.
	(df_urec_init): Ditto.
	(df_urec_local_finalize): Ditto.
	(df_ri_bb_compute): Added detection of last use of pseudos.
	* Makefile.in (df-scan.o): Updated dependencies.
	* config/mips/mips-protos.h (mips_set_live_on_entry): Added.
	* config/mips/mips.c (mips_set_live_on_entry): Added.
	* config/mips/mips.c (TARGET_EXTRA_LIVE_ON_ENTRY): Added value
	for target hook.
	* dce.c (marked_insn_p): Added code to handle artifical defs.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/Makefile.in
    trunk/gcc/bitmap.h
    trunk/gcc/config/mips/mips-protos.h
    trunk/gcc/config/mips/mips.c
    trunk/gcc/coretypes.h
    trunk/gcc/df-core.c
    trunk/gcc/df-problems.c
    trunk/gcc/df-scan.c
    trunk/gcc/df.h
    trunk/gcc/doc/tm.texi
    trunk/gcc/target-def.h
    trunk/gcc/target.h
    trunk/gcc/targhooks.c
    trunk/gcc/targhooks.h

Comment 19 Zdenek Dvorak 2006-02-09 22:34:27 UTC
Subject: Bug 24762

Author: rakdver
Date: Thu Feb  9 22:34:23 2006
New Revision: 110815

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=110815
Log:
	PR rtl-optimization/24762
	* df-scan.c (df_bb_refs_record): Record correct registers defined on
	eh edges.


Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/df-scan.c

Comment 20 Zdenek Dvorak 2006-02-09 22:34:48 UTC
Fixed.