This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] insn priority adjustments in scheduler and rs6000 port
Hi, Dorit. I have some comments about the patch. They are mostly
about coding style (absence comments for some global variables and
absence blanks before left parentheses in macro calls) and Changelog
entries (like absence periods at the end of sentences and usage of
lower case letters for the first word of sentences). Please, read
GNU coding standards. Also extern declarations should be in *.h files
(like sched_max_insns_priority should be sched-int.h or rtl.h).
If you create new compiler options, they should be described in the
documentation. Another things is that the patch is about improving
the generated code. Then we should have some benchmarks (the best
ones are SPEC) proving that the patch improves the code.
But in general the patch looks ok. So if you fix all comments and
give us some benchmarks, the patch could be committed in the mainline.
As for the patch about scheduling of queued insns, it is more
controversial. The same comments are applicable to the patch. But I
need to think more about the patch idea because it violates the
scheduler design:
1. insns are issued only from ready.
2. queue contains insns which can not be issued on the
current cycle (because of the data or resource delay).
Vlad
> 2003-09-09 Dorit Naishlos <dorit@il.ibm.com>
>
> * sched-rgn.c (init_ready_list): add invocations to
^ it should be capital letter.
The same below.
> targetm.sched.adjust_priority.
> * haifa-sched.c (sched_max_insns_priority): new global
> variable. Defined here, used in config/rs6000/rs6000.c.
^ additional blank.
> * config/rs6000/rs6000.h:
> (rs6000_sched_restricted_insns_priority_str): support new
> flag -mprioritize-restricted-insns.
> (DEFAULT_RESTRICTED_INSNS_PRIORITY): Define.
> * config/rs6000/rs6000.c (is_dispatch_slot_restricted): New
> function.
> (rs6000_adjust_priority): change priority of restricted
> insns, using above new function and new flag.
>
>
> (We've had problems with line breaks in the past, so I'm sending the patch
> both inlined and as an attachment)
>
> thanks,
> dorit
>
> (See attached file: sched1_patch)
>
> Index: gcc/gcc/haifa-sched.c
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/gcc/haifa-sched.c,v
> retrieving revision 1.228
> diff -c -3 -p -r1.228 haifa-sched.c
> *** gcc/gcc/haifa-sched.c 19 Aug 2003 23:21:59 -0000 1.228
> --- gcc/gcc/haifa-sched.c 7 Sep 2003 12:59:16 -0000
> *************** static int max_issue (struct ready_list
> *** 531,536 ****
> --- 531,538 ----
>
> static rtx choose_ready (struct ready_list *);
>
> + int sched_max_insns_priority = 0;
The global variable should have a comment.
> +
> #endif /* INSN_SCHEDULING */
>
> /* Point to state used for the current scheduling pass. */
> *************** set_priorities (rtx head, rtx tail)
> *** 2526,2531 ****
> --- 2528,2534 ----
> return 0;
>
> n_insn = 0;
> + sched_max_insns_priority = 0;
> for (insn = tail; insn != prev_head; insn = PREV_INSN (insn))
> {
> if (GET_CODE (insn) == NOTE)
> *************** set_priorities (rtx head, rtx tail)
> *** 2533,2539 ****
> --- 2536,2547 ----
>
> n_insn++;
> (void) priority (insn);
> +
> + if (INSN_PRIORITY_KNOWN(insn))
^ additional blank (the same
is in many places)
> + sched_max_insns_priority =
> + MAX (sched_max_insns_priority, INSN_PRIORITY(insn));
> }
> + sched_max_insns_priority += 1;
>
> return n_insn;
> }
> Index: gcc/gcc/sched-rgn.c
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/gcc/sched-rgn.c,v
> retrieving revision 1.63
> diff -c -3 -p -r1.63 sched-rgn.c
> *** gcc/gcc/sched-rgn.c 19 Jul 2003 22:03:37 -0000 1.63
> --- gcc/gcc/sched-rgn.c 7 Sep 2003 12:59:18 -0000
> *************** init_ready_list (struct ready_list *read
> *** 1756,1762 ****
> for (insn = NEXT_INSN (prev_head); insn != next_tail; insn = NEXT_INSN (insn))
> {
> if (INSN_DEP_COUNT (insn) == 0)
> ! ready_add (ready, insn);
> target_n_insns++;
> }
>
> --- 1756,1768 ----
> for (insn = NEXT_INSN (prev_head); insn != next_tail; insn = NEXT_INSN (insn))
> {
> if (INSN_DEP_COUNT (insn) == 0)
> ! {
> ! ready_add (ready, insn);
> !
> ! if (targetm.sched.adjust_priority)
> ! INSN_PRIORITY (insn) =
> ! (*targetm.sched.adjust_priority) (insn, INSN_PRIORITY (insn));
> ! }
> target_n_insns++;
> }
>
> *************** init_ready_list (struct ready_list *read
> *** 1792,1798 ****
> && check_live (insn, bb_src)
> && is_exception_free (insn, bb_src, target_bb))))
> if (INSN_DEP_COUNT (insn) == 0)
> ! ready_add (ready, insn);
> }
> }
> }
> --- 1798,1810 ----
> && check_live (insn, bb_src)
> && is_exception_free (insn, bb_src, target_bb))))
> if (INSN_DEP_COUNT (insn) == 0)
> ! {
> ! ready_add (ready, insn);
> !
> ! if (targetm.sched.adjust_priority)
> ! INSN_PRIORITY (insn) =
> ! (*targetm.sched.adjust_priority) (insn, INSN_PRIORITY (insn));
> ! }
> }
> }
> }
> Index: gcc/gcc/config/rs6000/rs6000.c
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/gcc/config/rs6000/rs6000.c,v
> retrieving revision 1.512
> diff -c -3 -p -r1.512 rs6000.c
> *** gcc/gcc/config/rs6000/rs6000.c 4 Sep 2003 03:17:58 -0000 1.512
> --- gcc/gcc/config/rs6000/rs6000.c 7 Sep 2003 12:59:26 -0000
> *************** struct rs6000_cpu_select rs6000_select[3
> *** 80,85 ****
> --- 80,91 ----
> { (const char *)0, "-mtune=", 1, 0 },
> };
>
> + /* Support adjust_priority scheduler hook
> + and -mprioritize-restricted-insns= option */
^ period and additional
blank
(the same in many
places)
> + const char *rs6000_sched_restricted_insns_priority_str;
> + int rs6000_sched_restricted_insns_priority;
> + extern int sched_max_insns_priority;
The extern variable should be in a .h file.
> +
> /* Size of long double */
> const char *rs6000_long_double_size_string;
> int rs6000_long_double_type_size;
> *************** static int rs6000_get_some_local_dynamic
> *** 322,327 ****
> --- 328,335 ----
> static rtx rs6000_complex_function_value (enum machine_mode);
> static rtx rs6000_spe_function_arg (CUMULATIVE_ARGS *, enum machine_mode, tree);
>
> + static bool is_dispatch_slot_restricted (rtx);
> +
> /* Hash table stuff for keeping track of TOC entries. */
>
> struct toc_hash_struct GTY(())
> *************** rs6000_override_options (default_cpu)
> *** 824,829 ****
> --- 832,843 ----
> rs6000_default_long_calls = (base[0] != 'n');
> }
>
> + /* Handle -mprioritize-restrcted-insns option */
> + rs6000_sched_restricted_insns_priority = DEFAULT_RESTRICTED_INSNS_PRIORITY;
> + if (rs6000_sched_restricted_insns_priority_str)
> + rs6000_sched_restricted_insns_priority =
> + atoi (rs6000_sched_restricted_insns_priority_str);
> +
> #ifdef TARGET_REGNAMES
> /* If the user desires alternate register names, copy in the
> alternate names now. */
> *************** rs6000_adjust_cost (insn, link, dep_insn
> *** 13353,13361 ****
> return cost;
> }
>
> /* A C statement (sans semicolon) to update the integer scheduling
> ! priority INSN_PRIORITY (INSN). Reduce the priority to execute the
> ! INSN earlier, increase the priority to execute INSN later. Do not
> define this macro if you do not need to adjust the scheduling
> priorities of insns. */
>
> --- 13367,13413 ----
> return cost;
> }
>
> + /* Return 1 if INSN can be scheduled only as the first insn
> + in a dispatch group. Retrun 0 otherwise. */
> +
> + static bool
> + is_dispatch_slot_restricted (rtx insn)
> + {
> + enum attr_type type;
> +
> + if (rs6000_cpu != PROCESSOR_POWER4)
> + return 0;
> +
> + if (!insn
> + || insn == NULL_RTX
> + || GET_CODE(insn) == NOTE
> + || GET_CODE (PATTERN (insn)) == USE
> + || GET_CODE (PATTERN (insn)) == CLOBBER)
> + return 0;
> +
> + type = get_attr_type (insn);
> +
> + switch (type){
> + case TYPE_MFCR:
> + case TYPE_MFCRF:
> + case TYPE_MTCR:
> + case TYPE_DELAYED_CR:
> + case TYPE_CR_LOGICAL:
> + case TYPE_MTJMPR:
> + case TYPE_MFJMPR:
> + return 1;
> + case TYPE_IDIV:
> + case TYPE_LDIV:
> + return 2;
> + default:
> + return 0;
> + }
> + }
> +
> +
> /* A C statement (sans semicolon) to update the integer scheduling
> ! priority INSN_PRIORITY (INSN). Increase the priority to execute the
> ! INSN earlier, reduce the priority to execute INSN later. Do not
> define this macro if you do not need to adjust the scheduling
> priorities of insns. */
>
> *************** rs6000_adjust_priority (insn, priority)
> *** 13393,13398 ****
> --- 13445,13469 ----
> }
> }
> #endif
> +
> + if (is_dispatch_slot_restricted (insn)
> + && reload_completed
> + && sched_max_insns_priority
> + && rs6000_sched_restricted_insns_priority)
> + {
> +
> + /* prioritize insns that can be dispatched only in the first dispatch slot */
> + if (rs6000_sched_restricted_insns_priority == 1)
> + /* Attach highest priority to insn. This means that in
> + haifa-sched.c:ready_sort(), dispatch-slot restriction considerations
> + precede 'priority' (critical path) considerations. */
^
additional
blank
> + return sched_max_insns_priority;
> + else if (rs6000_sched_restricted_insns_priority == 2)
> + /* Increase priority on insn by a minimal amount. This means that in
> + haifa-sched.c:ready_sort(), only 'priority' (critical path) considerations
> + precede dispatch-slot restriction considerations */
> + return (priority + 1);
> + }
>
> return priority;
> }
> Index: gcc/gcc/config/rs6000/rs6000.h
> ===================================================================
> RCS file: /cvsroot/gcc/gcc/gcc/config/rs6000/rs6000.h,v
> retrieving revision 1.286
> diff -c -3 -p -r1.286 rs6000.h
> *** gcc/gcc/config/rs6000/rs6000.h 21 Jul 2003 20:18:52 -0000 1.286
> --- gcc/gcc/config/rs6000/rs6000.h 7 Sep 2003 12:59:28 -0000
> *************** extern enum processor_type rs6000_cpu;
> *** 404,409 ****
> --- 404,411 ----
> {"no-longcall", &rs6000_longcall_switch, "", 0}, \
> {"align-", &rs6000_alignment_string, \
> N_("Specify alignment of structure fields default/natural"), 0}, \
> + {"prioritize-restricted-insns=", &rs6000_sched_restricted_insns_priority_str, \
> + N_("Specify scheduling priority for dispatch slot restricted insns"), 0}, \
In my opinion, it is better to use enumeration for the parameter
value. Because it is not just a value but some encoding.
> SUBTARGET_OPTIONS \
> }
>
> *************** extern const char *rs6000_longcall_switc
> *** 457,462 ****
> --- 459,466 ----
> extern int rs6000_default_long_calls;
> extern const char* rs6000_alignment_string;
> extern int rs6000_alignment_flags;
> + extern const char *rs6000_sched_restricted_insns_priority_str;
> + extern int rs6000_sched_restricted_insns_priority;
>
> /* Alignment options for fields in structures for sub-targets following
> AIX-like ABI.
> *************** extern int rs6000_alignment_flags;
> *** 474,479 ****
> --- 478,486 ----
> #else
> #define TARGET_ALIGN_NATURAL 0
> #endif
> +
> + /* Define if the target has restricted dispatch slot instructions */
> + #define DEFAULT_RESTRICTED_INSNS_PRIORITY (rs6000_cpu == PROCESSOR_POWER4 ? 1 : 0)
>
> /* Define TARGET_MFCRF if the target assembler supports the optional
> field operand for mfcr and the target processor supports the
>