[MIPS][LS2][4/5] Scheduling and tuning

[Richard Sandiford]

[Silly thing, but I tried applying these patches, and was alerted
to trailing whitespace in:

    gcc/testsuite/gcc.target/mips/loongson-simd.c
    gcc/config/mips/loongson.md
    gcc/config/mips/loongson2ef.md

Please run them through your favourite trailing-whitespace remover
before applying.]

Anyway, this patch looks good, thanks.  A neat use of CPU querying ;)

My usual handful of mind-numbing nits follow...

Maxim Kuvyrkov <maxim@codesourcery.com> writes:
> +;; Pseudo units to enable/disable ls2_falu[12]_turn units.
> +;; ls2_falu[12]_turn unit can be subscribed only after
> +;; ls2_falu[12]_turn_enabled unit is subscribed.
> +(define_cpu_unit "ls2_falu1_turn_enabled,ls2_falu2_turn_enabled"
> +  "ls2_falu")

Needless line break (inconsistent with integer/core stuff).

> +;; Subscribe ls2_alu1_turn_enabled.
> +(define_insn "ls2_alu1_turn_enabled_insn"
> +  [(unspec [(const_int 0)] UNSPEC_LOONGSON_ALU1_TURN_ENABLED_INSN)]
> +  "TUNE_LOONGSON_2EF"
> +{
> +  gcc_unreachable ();
> +  return "";
> +}

Plain:

  { gcc_unreachable (); }

ought to be OK.  gcc_unreachable() is noreturn.

> @@ -301,8 +335,14 @@ struct machine_function GTY(()) {
>    /* True if we have emitted an instruction to initialize
>       mips16_gp_pseudo_rtx.  */
>    bool initialized_mips16_gp_pseudo_p;
> +
> +  /* Data used when scheduling for Loongson 2E/2F.  */
> +  struct sched_ls2_def _sched_ls2;
>  };
>  
> +/* A convenient shortcut.  */
> +#define sched_ls2 (cfun->machine->_sched_ls2)
> +

Hmm, we really shouldn't use _foo names.

I don't see why this has to be in machine_function anyway.  It's
pass-local, so why not just use a static sched_ls2 variable, like
we do for other scheduling stuff?

(I assume putting this in machine_function keeps the fake insns
around between sched1 and sched2, even though we create new insns
during the initialisation phase of sched2.)

Actually, make that "ls2_sched" or "mips_ls2_sched", for consistency
with other ISA- or processor-prefixed names.  Same with with the other
similar names in the patch; sometimes you've used "sched_ls2_foo"
and sometimes you've used "mips_ls2_foo".  "mips_ls2_foo" is fine.

> +/* Implement TARGET_SCHED_INIT_DFA_POST_CYCLE_INSN hook.
> +   Init data used in mips_dfa_post_advance_cycle.  */
> +static void
> +mips_init_dfa_post_cycle_insn (void)

Local style is to have a blank line before the function.
Other instances.

> +{
> +  if (TUNE_LOONGSON_2EF)
> +    {
> +      start_sequence ();
> +      emit_insn (gen_ls2_alu1_turn_enabled_insn ());
> +      sched_ls2.alu1_turn_enabled_insn = get_insns ();
> +      end_sequence ();
> +
> +      start_sequence ();
> +      emit_insn (gen_ls2_alu2_turn_enabled_insn ());
> +      sched_ls2.alu2_turn_enabled_insn = get_insns ();
> +      end_sequence ();
> +
> +      start_sequence ();
> +      emit_insn (gen_ls2_falu1_turn_enabled_insn ());
> +      sched_ls2.falu1_turn_enabled_insn = get_insns ();
> +      end_sequence ();
> +
> +      start_sequence ();
> +      emit_insn (gen_ls2_falu2_turn_enabled_insn ());
> +      sched_ls2.falu2_turn_enabled_insn = get_insns ();
> +      end_sequence ();
> +
> +      sched_ls2.alu1_core_unit_code = get_cpu_unit_code ("ls2_alu1_core");
> +      sched_ls2.alu2_core_unit_code = get_cpu_unit_code ("ls2_alu2_core");
> +      sched_ls2.falu1_core_unit_code = get_cpu_unit_code ("ls2_falu1_core");
> +      sched_ls2.falu2_core_unit_code = get_cpu_unit_code ("ls2_falu2_core");
> +    }

Please put the "if" body in a separate ls2_init_dfa_post_cycle_insn
function, for consistency with other hooks.  (You did this for
sched_ls2_dfa_post_advance_cycle, thanks.)

> @@ -9982,6 +10129,14 @@ mips_sched_init (FILE *file ATTRIBUTE_UN
>    mips_macc_chains_last_hilo = 0;
>    vr4130_last_insn = 0;
>    mips_74k_agen_init (NULL_RTX);
> +
> +  if (TUNE_LOONGSON_2EF)
> +    {
> +      /* Branch instructions go to ALU1, therefore basic block is most likely
> + 	 to start with round-robin counter pointed to ALU2.  */
> +      sched_ls2.alu1_turn_p = false;
> +      sched_ls2.falu1_turn_p = true;
> +    }

As you can see from the context, we initialise other schedulers'
information unconditionally.  That's amenable to change if you like,
but whatever we do, let's be consistent.

> @@ -10022,6 +10204,21 @@ mips_variable_issue (FILE *file ATTRIBUT
>        vr4130_last_insn = insn;
>        if (TUNE_74K)
>  	mips_74k_agen_init (insn);
> +      else if (TUNE_LOONGSON_2EF)
> +	{
> +	  mips_ls2_variable_issue ();
> +
> +	  if (recog_memoized (insn) >= 0)
> +	    {
> +	      sched_ls2.cycle_has_multi_p |= (get_attr_type (insn)
> +					      == TYPE_MULTI);
> +
> +	      /* Instructions of type 'multi' should all be split before
> +		 second scheduling pass.  */
> +	      gcc_assert (!sched_ls2.cycle_has_multi_p
> +			  || !reload_completed);
> +	    }
> +	}

Please put all the Loongson bits in mips_ls2_variable_issue.

I agree that the assert is a good thing, but I think we should require
it for all targets or none.  If we don't, it's too likely that someone
working on another target will accidentally break Loongson.

Richard