Re: Ping: Unreviewed patch (x86-64: Add support for non temporal prefetches)

[Steven Bosscher <stevenb at suse dot de>]

On Friday 10 September 2004 13:10, Nutan Singh wrote:
> Hi,
>
> Here is the patch for GCC-3.5 mainline. Please review it.

> +        * loop.c (emit_prefetch_instructions): Emit non-temporal prefetches
> +	where givs are not reused.
> +	(non_temporal_store): New function for generating non-temporal stores.

loop.c and the prefetching stuff in it will go away soon,
it really is not very useful to apply a patch to it now.
You could either revive loop-prefetch.c from the rtlopt
branch, or work on the lno-branch to implement this kind
of prefetching for trees.  I suggest the latter.


> +	* cfgloop.c (remove_redundant_prefetches): New function to remove
> +	redundant prefetches for a loop to avoid fetching same cache line
> +	repeatedly.

This function would probably be useful even without the
non-temporal prefetch hunks for loop.c, but I do have some
comments on your code.


> +#ifdef HAVE_prefetch
> +void
> +remove_redundant_prefetches (struct loops *loops, FILE * rtl_dump_file)
> +{
> +  basic_block *bbs;
> +  rtx plist = 0, insn;
> +  int i, j;

Where is the comment before the function declaration?  The
GCC coding style conventions require that you add a comment
for each function describing the purpose of the function
and the arguments that the function takes.


> +		  plist = gen_rtx_EXPR_LIST (VOIDmode, addr, plist);

Is an EXPR_LIST really the best data structure here?  Not
that it would matter much, but I would like us to be more
careful about generating garbage RTL like this.  Perhaps a
hash tab or some local list structure that does not live in
GGC space?


This hunk,
> +   case OPT_fguess_loop_iteration:
> +     set_param_value ("assumed-loop-iteration", value);
> +     break;

together with this,
> +fguess-loop-iteration
> +Common
> +Enable guessing of loop iteration count

will result in setting your assumed-loop-iteration value
to 1.  You should look at finline-limit= in common.opts to
see how you can do what you are trying to do.
Oh, and feel free to kill the max-inline-insns-rtl param
while you're at it, please ;-)  It's not used anymore since
we don't inline RTL anymore.


> +#ifdef m_ATHLON_K8
> +const struct processor_costs *ix86_cost = &k8_cost;
> +#else
> +const struct processor_costs *ix86_cost = &pentium_cost;
> +#endif

This hunk doesn't have much to do with the non-temporal
prefetching parts of your patch.  Also, it is wrong since
m_ATHLON_K8 is always defined for all i386 targets.

Gr.
Steven