Re: [PATCH, SPU] Fix PR47179 (Re: [PATCH] Prepare for a fix for PR47179)

[Richard Guenther <rguenther at suse dot de>]

On Tue, 18 Jan 2011, Ulrich Weigand wrote:

> Richard Guenther wrote:
> 
> > This makes it possible to specify a target specific way to disambiguate
> > against errno.  SPU has a particular bad way that isn't caught by
> > the generic implementation (and I'll leave it to the target maintainer
> > to implement one, the most trivial one just returning true always).
> 
> Thanks!  Using this patch and the one below fixes the test case on SPU.
> 
> Before I commit the SPU patch, I was wondering if you had any hints or
> suggestions how to tighten up the check.  Since our errno is part of
> a structure, we actually have to mark the whole *structure* as aliased
> with errno.  Thus the patch currently simply considers *any* global
> external variable of structure type to potentially alias errno.
> 
> Should this maybe tightened down to just structure types that contain
> an "int"?  Maybe even just those with an "int" as first member?  It
> would probably not be a good idea to actually check for the specific
> struct tag name ...
> 
> On the other hand, it might not be really critical either way.
> 
> 
> Patch tested on spu-elf with no regression.
> 
> Bye,
> Ulrich
> 
> ChangeLog:
> 
> 	PR tree-optimization/47179
> 	* config/spu/spu.c (spu_ref_may_alias_errno): New function.
> 	(TARGET_REF_MAY_ALIAS_ERRNO): Define.
> 
> 
> Index: gcc/config/spu/spu.c
> ===================================================================
> *** gcc/config/spu/spu.c	(revision 168909)
> --- gcc/config/spu/spu.c	(working copy)
> *************** static void spu_unique_section (tree, in
> *** 230,235 ****
> --- 230,236 ----
>   static rtx spu_expand_load (rtx, rtx, rtx, int);
>   static void spu_trampoline_init (rtx, tree, rtx);
>   static void spu_conditional_register_usage (void);
> + static bool spu_ref_may_alias_errno (ao_ref *);
>   
>   /* Which instruction set architecture to use.  */
>   int spu_arch;
> *************** static const struct attribute_spec spu_a
> *** 491,496 ****
> --- 492,500 ----
>   #undef TARGET_CONDITIONAL_REGISTER_USAGE
>   #define TARGET_CONDITIONAL_REGISTER_USAGE spu_conditional_register_usage
>   
> + #undef TARGET_REF_MAY_ALIAS_ERRNO
> + #define TARGET_REF_MAY_ALIAS_ERRNO spu_ref_may_alias_errno
> + 
>   struct gcc_target targetm = TARGET_INITIALIZER;
>   
>   static void
> *************** spu_function_profiler (FILE * file, int 
> *** 7150,7153 ****
> --- 7154,7179 ----
>     fprintf (file, "brsl $75,  _mcount\n");
>   }
>   
> + /* Implement targetm.ref_may_alias_errno.  */
> + static bool
> + spu_ref_may_alias_errno (ao_ref *ref)
> + {
> +   tree base = ao_ref_base (ref);
> + 
> +   /* With SPU newlib, errno is defined as something like
> +          _impure_data._errno
> +      The default implementation of this target macro does not
> +      recognize such expressions.
> + 
> +      To fix this, we assume here that any access to a global variable
> +      of structure type may alias errno.  */
> + 
> +   if (DECL_P (base)

TREE_CODE (base) == VAR_DECL

> +       && !TREE_STATIC (base)

As you are checking for a RECORD_TYPE you need to exclude auto-variables
like with

	  && DECL_EXTERNAL (base)

> +       && TREE_CODE (TREE_TYPE (base)) == RECORD_TYPE)
> +     return true;
> + 
> +   return default_ref_may_alias_errno (ref);

I think you can safely remove the default_ref_may_alias_errno call
and return false.

Btw, you could match the DECL_ASSEMBLER_NAME with _impure_data
(newlib is not thread safe?) and even match the offset of the
errno member if you like (ao_ref_offset (ref)).

Otherwise the patch looks sensible to me.
Richard.