[003/236] config/mn10300: Fix missing PATTERN in PARALLEL handling

On Wed, 2014-08-27 at 09:11 -0700, Richard Henderson wrote:
> On 08/27/2014 08:48 AM, David Malcolm wrote:
> > Alternatively, should this simply use "single_set"?
> 
> Yes.
> 
> > (though I think that's a more invasive change, especially since some of
> > the logic is for non-SETs).
> 
> I don't think that's the case.  Take the tests in order:
> 
>   if (mn10300_tune_cpu == PROCESSOR_AM34
>       && is_load_insn (dep)
>       && is_store_insn (insn))
>     cost += 1;
> 
> Requires sets for both.
> 
>   else if (mn10300_tune_cpu == PROCESSOR_AM34
>       && ! is_store_insn (insn)
>       && ! JUMP_P (insn)
>       && GET_CODE (PATTERN (dep)) == SET
>       && GET_CODE (PATTERN (insn)) == SET
> 
> Duh.
> 
>   if (GET_CODE (PATTERN (dep)) != SET)
>     return cost;
> 
> Filtering out non-sets from dep.
> 
>   /* Now check to see if the previous instruction is a load or store.  */
>   if (! is_load_insn (insn) && ! is_store_insn (insn))
>     return cost;
> 
> Filtering out non-sets from insn.
> 
> Thus in no case do we return anything but the original "cost" when either the
> dep or insn pattern is not a set.
> 
> Oh, and while you're massaging this function...
> 
> mn10300_adjust_sched_cost (rtx insn, rtx link, rtx dep, int cost)
> {
>   int timings = get_attr_timings (insn);
> ...
>   /* Extract the latency value from the timings attribute.  */
>   return timings < 100 ? (timings % 10) : (timings % 100);
> }
> 
> 
> Will you please move the (expensive) get_attr_timings call to the end, after
> we've discarded all of the cases in which it isn't used?

Fair enough.

Update version of patch attached; again, only lightly tested so far.

	* gcc/config/mn10300/mn10300.c (is_load_insn): Rename to...
	(set_is_load_p): ...this, updating to work on a SET pattern rather
	than an insn.
	(is_store_insn): Rename to...
	(set_is_store_p): ...this, updating to work on a SET pattern
	rather than an insn.
	(mn10300_adjust_sched_cost): Move call to get_attr_timings from
	top of function to where it is needed.  Rewrite the bogus
	condition that checks for "insn" and "dep" being PARALLEL to
	instead use single_set, introducing locals "insn_set" and
	"dep_set".  Given that we only ever returned "cost" for a non-pair
	of SETs, bail out early if we don't have a pair of SET.
	Rewrite all uses of PATTERN (dep) and PATTERN (insn) to instead
	use the new locals "insn_set" and "dep_set", and update calls to
	is_load_insn and is_store_insn to be calls to set_is_load_p and
	set_is_store_p.

On 08/27/2014 09:32 AM, David Malcolm wrote:
> 	* gcc/config/mn10300/mn10300.c (is_load_insn): Rename to...
> 	(set_is_load_p): ...this, updating to work on a SET pattern rather
> 	than an insn.
> 	(is_store_insn): Rename to...
> 	(set_is_store_p): ...this, updating to work on a SET pattern
> 	rather than an insn.
> 	(mn10300_adjust_sched_cost): Move call to get_attr_timings from
> 	top of function to where it is needed.  Rewrite the bogus
> 	condition that checks for "insn" and "dep" being PARALLEL to
> 	instead use single_set, introducing locals "insn_set" and
> 	"dep_set".  Given that we only ever returned "cost" for a non-pair
> 	of SETs, bail out early if we don't have a pair of SET.
> 	Rewrite all uses of PATTERN (dep) and PATTERN (insn) to instead
> 	use the new locals "insn_set" and "dep_set", and update calls to
> 	is_load_insn and is_store_insn to be calls to set_is_load_p and
> 	set_is_store_p.

Ok, if it passes your smoke tests.

> +  /* We are only interested in pairs of SET. */
> +  insn_set = single_set (insn);
> +  if (!insn_set)
> +    return cost;
>  
> +  dep_set = single_set (dep);
> +  if (!dep_set)
> +    return cost;
>  
> +  gcc_assert (GET_CODE (insn_set) == SET);
> +  gcc_assert (GET_CODE (dep_set) == SET);

I don't think you need the asserts; we should be able to trust single_set.


r~