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

[Maxim Kuvyrkov]

Richard Sandiford wrote:
> [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.]

Thanks for detailed review.  I've attached update patch with all issues 
fixed.  I've also added a couple more things in it:

* Catch-all reservation in loongson2ef.md.  This is the last reservation 
in the file, please check that you are happy with the comment ;)

* (define_attr "cpu"): I've renamed values loongson2? to loongson_2?. 
This corrects a typo in [1/5] patch.


I've fixed all the formatting issues.  Thanks for pointing them out.

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

Thanks.

...

>> @@ -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.)

I've made it struct { ... } mips_ls2;.

> 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.

The rationale here was that exported stuff (like hooks) have 
mips_ls2_sched_* names while static variables and static functions are 
named sched_ls2_*.  I've changed all names to mips_ls2.

> 
>> +/* 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.

Fixed.

...

> 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.)

Fixed

> 
>> @@ -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.

The initialization is unconditional now.

> 
>> @@ -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.

I've put loongson bits to mips_ls2_variable_issue and made the assert 
work for all architectures.


--
Maxim
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: fsf-ls2ef-4-sched.ChangeLog
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20080612/c1b61797/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: fsf-ls2ef-4-sched.patch
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20080612/c1b61797/attachment-0001.ksh>