RFC: default insn length should be 1 but that breaks m68hc11 and regresses mn10300.

Hans-Peter Nilsson hans-peter.nilsson@axis.com
Sat Apr 2 09:09:00 GMT 2005


All this with LAST_UPDATED: "Tue Mar 29 15:04:38 UTC 2005".

When changing to RTL epilogue for CRIS, I noticed a 5% bloat and
a swing in +-1% performance (mostly on the pessimization side
though).  I would have expected only performance improvement and
if any code size increase, a very small such (the epilogue being
copied, on the other hand its delay-slot filled).  It seems the
bb-reorder pass is guilty: it copies and moves around code,
using the size as a metric and it relies on get_insn_length ()
in final to tell it the size of an insn.  Unfortunately, the
default size (when not defining attribute "length" in the .md)
is 0.  It does not increase while accumulating...

Here's a patch to change the default to 1; I can't think of a
better default value.  asm_insn_count doesn't depend on
HAVE_ATTR_length, so I don't see the point in the #ifdef
HAVE_ATTR_length there, unless to cover that all other insns
have zero length, of course.

For CRIS, this has a consistent 6-8% code size *decreasing*
effect (embedded folks really like that effect), while mostly
performance-neutral: figures swing in the +-0.5% range and it
seems I can blame most of the performance changes on the reorg
(delayed-branch) pass.

Maintainers for ports mentioned below will likely be interested.

	* final.c (asm_insn_count): Do not gate with #ifdef
	HAVE_ATTR_length.
	(get_attr_length) [! HAVE_ATTR_length]: Use 1, not 0, as default
	length.

Index: final.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/final.c,v
retrieving revision 1.346
diff -c -p -r1.346 final.c
*** final.c	23 Mar 2005 15:59:38 -0000	1.346
--- final.c	2 Apr 2005 08:27:00 -0000
*************** static int dialect_number;
*** 212,220 ****
  rtx current_insn_predicate;
  #endif
  
- #ifdef HAVE_ATTR_length
  static int asm_insn_count (rtx);
- #endif
  static void profile_function (FILE *);
  static void profile_after_prologue (FILE *);
  static bool notice_source_line (rtx);
--- 212,218 ----
*************** init_insn_lengths (void)
*** 380,386 ****
  int
  get_attr_length (rtx insn ATTRIBUTE_UNUSED)
  {
! #ifdef HAVE_ATTR_length
    rtx body;
    int i;
    int length = 0;
--- 378,387 ----
  int
  get_attr_length (rtx insn ATTRIBUTE_UNUSED)
  {
! #ifndef HAVE_ATTR_length
! #define insn_default_length(INSN) 1
! #endif
! 
    rtx body;
    int i;
    int length = 0;
*************** get_attr_length (rtx insn ATTRIBUTE_UNUS
*** 432,440 ****
    ADJUST_INSN_LENGTH (insn, length);
  #endif
    return length;
- #else /* not HAVE_ATTR_length */
-   return 0;
- #endif /* not HAVE_ATTR_length */
  }
  
  /* Code to handle alignment inside shorten_branches.  */
--- 433,438 ----
*************** shorten_branches (rtx first ATTRIBUTE_UN
*** 1299,1305 ****
  #endif /* HAVE_ATTR_length */
  }
  
- #ifdef HAVE_ATTR_length
  /* Given the body of an INSN known to be generated by an ASM statement, return
     the number of machine instructions likely to be generated for this insn.
     This is used to compute its length.  */
--- 1297,1302 ----
*************** asm_insn_count (rtx body)
*** 1321,1327 ****
  
    return count;
  }
- #endif
  
  /* Output assembler code for the start of a function,
     and initialize some of the variables in this file
--- 1318,1323 ----

Unfortunately this patch causes regressions for some targets
without attribute "length": m68hc11-elf doesn't build and
mn10300-elf has 6 libstdc++-v3 regressions.  MMIX is fine (well,
of course ;-)  I can't test the other non-attr-length targets:
ns32k, m68k, vax.  Populate src/sim and bring newlib ports, you!

For m68hc11, it's a SEGV building newlib (the -m68hc12 -mshort
multilib building newlib/libc/stdio/getdelim.c), caused by
dereferencing NULL.  It happens in m68hc11_reorg calling
compute_bb_for_insn, where it seems BB_END of a block points to
a deleted insn which is not in the chain linked from BB_HEAD
(causing the loop to run into the NULL at the end of insns):

(gdb) p bb
$11 = 0x400d7438
(gdb) pbb
;; basic block 13, loop depth 0, count 0
;; prev block 12, next block 14
;; pred:       12 [89.6%]  (fallthru,can_fallthru)
;; succ:       14 [100.0%]  (fallthru,can_fallthru)
;; Registers live at start:  3 [sp] 9 [*_.frame] 46 [*sframe]
(note:HI 133 360 134 13 [bb 13] NOTE_INSN_BASIC_BLOCK)
(note:HI 134 133 270 13 ("/home/hp/cvs_areas/combined/combined/newlib/libc/stdio/getdelim.c") 139)
(insn 270 134 271 13 /home/hp/cvs_areas/combined/combined/newlib/libc/stdio/getdelim.c:139 (set (reg:HI 2 y)
        (reg/f:HI 9 *_.frame)) 26 {*movhi_68hc12} (nil)
    (nil))
(insn 271 270 135 13 /home/hp/cvs_areas/combined/combined/newlib/libc/stdio/getdelim.c:139 (set (reg:HI 2 y)
        (mem:HI (plus:HI (reg:HI 2 y)
                (const_int 4 [0x4])) [53 ptr+0 S2 A8])) 26 {*movhi_68hc12} (nil)
    (nil))
(insn:HI 135 271 136 13 /home/hp/cvs_areas/combined/combined/newlib/libc/stdio/getdelim.c:139 (set (mem:QI (reg:HI 2 y) [0 S1 A8]\
)
        (const_int 0 [0x0])) 28 {movqi_const0} (nil)
    (nil))
(note:HI 136 135 273 13 ("/home/hp/cvs_areas/combined/combined/newlib/libc/stdio/getdelim.c") 140)
(insn 273 136 272 13 /home/hp/cvs_areas/combined/combined/newlib/libc/stdio/getdelim.c:140 (set (reg:HI 2 y [72])
        (reg:HI 2 y)) 26 {*movhi_68hc12} (nil)
    (expr_list:REG_DEAD (reg:HI 8 z)
        (nil)))
(insn 272 273 137 13 /home/hp/cvs_areas/combined/combined/newlib/libc/stdio/getdelim.c:140 (set (reg:HI 0 x)
        (reg/f:HI 9 *_.frame)) 26 {*movhi_68hc12} (nil)
    (nil))
(insn:HI 137 272 398 13 /home/hp/cvs_areas/combined/combined/newlib/libc/stdio/getdelim.c:140 (set (reg:HI 2 y [72])
        (minus:HI (reg:HI 2 y [72])
            (mem:HI (plus:HI (reg:HI 0 x)
                    (const_int 10 [0xa])) [56 buf.14+0 S2 A8]))) 60 {*subhi3} (nil)
    (expr_list:REG_DEAD (reg:HI 0 x)
        (nil)))
;; Registers live at end:  0 [x] 1 [d] 3 [sp] 9 [*_.frame] 46 [*sframe]
(gdb) p end
$12 = 0x400f31b8
(gdb) pr
(note/v 275 137 398 NOTE_INSN_DELETED)

It seems m68hc11-elf is in a shoddy shape at the moment, given
these baseline numbers (and you need to edit the baseboard file
which references the nonexistent sim-valid.x script, replacing
with I guess sim-valid-m68hc11.ld):

                === gcc Summary ===

# of expected passes            22265
# of unexpected failures        5618
# of unexpected successes       1
# of expected failures          74
# of unresolved testcases       4754
# of untested testcases         28
# of unsupported tests          600
/home/hp/cvs_areas/combined/m68hc11-regobj/gcc/xgcc  version 4.1.0 20050329 (experimental)

I'm assuming the port is to blame, but I won't insist on
"killing it" with the patch.  For mn10300-elf, the patch gives
these regressions:

+libstdc++.sum 22_locale/locale/cons/12352.cc
+libstdc++.sum 23_containers/bitset/cons/1.cc
+libstdc++.sum 27_io/basic_filebuf/imbue/12206.cc
+libstdc++.sum 27_io/basic_istream/extractors_arithmetic/char/exceptions_badbit_throw.cc
+libstdc++.sum 27_io/basic_stringbuf/snextc/char/1.cc
+libstdc++.sum 27_io/basic_stringbuf/str/char/1.cc

Those are execution failures.  I haven't investigated further,
as, ehm, execution failure investigations are prone to require
noticeable amounts of time, and I'm not anyway take this patch
anywhere.

I just ask for random insight and clues and presents this as a
heads-up to the maintainers of ports mentioned above.

I'm going weasel out of the related bug-swamp and instead
(define_attr "length" "" (const_int 2)) for CRIS.  Defining it
(but more correctly) is on the TODO list anyway for use in the
shorten-branches pass.

brgds, H-P



More information about the Gcc-patches mailing list