This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][RFC] "Fix" PR50716, override type alignment knowledge


On Mon, 17 Oct 2011, Eric Botcazou wrote:

> > 2011-10-17  Richard Guenther  <rguenther@suse.de>
> >
> > 	PR middle-end/50716
> > 	* expr.c (get_object_or_type_alignment): New function.
> > 	(expand_assignment): Use it.
> > 	(expand_expr_real_1): Likewise.
> 
> Maybe move it to builtins.c alongside the other get_*_alignment functions.

I explicitly didn't do that because the function shouldn't be used
without knowing exactly what you do ;)

> > Index: gcc/expr.c
> > ===================================================================
> > *** gcc/expr.c	(revision 180077)
> > --- gcc/expr.c	(working copy)
> > *************** get_bit_range (unsigned HOST_WIDE_INT *b
> > *** 4544,4549 ****
> > --- 4544,4568 ----
> >       }
> >   }
> >
> > + /* Return the alignment of the object EXP, also considering its type
> > +    when we do not know of explicit misalignment.
> > +    ???  Note that generally the type of an expression is not kept
> > +    consistent with misalignment information by the frontend, for
> > +    example when taking the address of a member of a packed structure.
> > +    So taking into account type information for alignment is generally
> > +    wrong, but is done here as a compromise.  */
> 
> This sounds backwards, as taking into account type information is generally 
> correct, i.e. the packed case is the exception.  Maybe use "in the general 
> case" instead:
> 
>   ??? Note that, in the general case, the type of an expression is not kept
>   consistent with the misalignment by the front-end, for example when taking
>   the address of a member of a packed structure.  However, in most of the
>   cases, expressions have the alignment of their type, so we fall back to
>   the alignment of the type when we cannot compute a misalignment.

Ok, I'll adjust it a bit, adding "so we fall back optimistically to
the alignment...", because that's what is true - for the packed case
we simply compute wrong alignment this way (but for the expand case
this has been the case since forever, noticed only on strict-align
targets).

Note that "cannot compute a misalignment" applies also to the
case where we for some reason explicitly know the alignment
is just BITS_PER_UNIT (you cannot 'misalign' that).  So there are
still a lot of cases where known (in some sense of known) alignment
is overridden by maybe bogus type information.  It's just less
cases now, but I think we may be able to improve the situation
by maybe returning an extra argument from get_object_alignment_1
that says whether the alignment might be bigger or whether the
alignment is definitively known.  But that's for a followup I
presume.

> 
> + static unsigned int
> + get_object_or_type_alignment (tree exp)
> + {
> +   unsigned HOST_WIDE_INT misalign;
> +   unsigned int align = get_object_alignment_1 (exp, &misalign);
> +   align = MAX (TYPE_ALIGN (TREE_TYPE (exp)), align);
> +   if (misalign != 0)
> +     align = (misalign & -misalign);
> 
> You don't need to go through the MAX if misalign is non-zero.

Ok.

Committed as follows.

Richard.

2011-10-18  Richard Guenther  <rguenther@suse.de>

	PR middle-end/50716
	* expr.c (get_object_or_type_alignment): New function.
	(expand_assignment): Use it.
	(expand_expr_real_1): Likewise.

Index: gcc/expr.c
===================================================================
*** gcc/expr.c	(revision 180077)
--- gcc/expr.c	(working copy)
*************** get_bit_range (unsigned HOST_WIDE_INT *b
*** 4544,4549 ****
--- 4544,4570 ----
      }
  }
  
+ /* Return the alignment of the object EXP, also considering its type
+    when we do not know of explicit misalignment.
+    ???  Note that, in the general case, the type of an expression is not kept
+    consistent with misalignment information by the front-end, for
+    example when taking the address of a member of a packed structure.
+    However, in most of the cases, expressions have the alignment of
+    their type, so we optimistically fall back to the alignment of the
+    type when we cannot compute a misalignment.  */
+ 
+ static unsigned int
+ get_object_or_type_alignment (tree exp)
+ {
+   unsigned HOST_WIDE_INT misalign;
+   unsigned int align = get_object_alignment_1 (exp, &misalign);
+   if (misalign != 0)
+     align = (misalign & -misalign);
+   else
+     align = MAX (TYPE_ALIGN (TREE_TYPE (exp)), align);
+   return align;
+ }
+ 
  /* Expand an assignment that stores the value of FROM into TO.  If NONTEMPORAL
     is true, try generating a nontemporal store.  */
  
*************** expand_assignment (tree to, tree from, b
*** 4553,4559 ****
    rtx to_rtx = 0;
    rtx result;
    enum machine_mode mode;
!   int align;
    enum insn_code icode;
  
    /* Don't crash if the lhs of the assignment was erroneous.  */
--- 4574,4580 ----
    rtx to_rtx = 0;
    rtx result;
    enum machine_mode mode;
!   unsigned int align;
    enum insn_code icode;
  
    /* Don't crash if the lhs of the assignment was erroneous.  */
*************** expand_assignment (tree to, tree from, b
*** 4571,4578 ****
    if ((TREE_CODE (to) == MEM_REF
         || TREE_CODE (to) == TARGET_MEM_REF)
        && mode != BLKmode
!       && ((align = MAX (TYPE_ALIGN (TREE_TYPE (to)), get_object_alignment (to)))
! 	  < (signed) GET_MODE_ALIGNMENT (mode))
        && ((icode = optab_handler (movmisalign_optab, mode))
  	  != CODE_FOR_nothing))
      {
--- 4592,4599 ----
    if ((TREE_CODE (to) == MEM_REF
         || TREE_CODE (to) == TARGET_MEM_REF)
        && mode != BLKmode
!       && ((align = get_object_or_type_alignment (to))
! 	  < GET_MODE_ALIGNMENT (mode))
        && ((icode = optab_handler (movmisalign_optab, mode))
  	  != CODE_FOR_nothing))
      {
*************** expand_expr_real_1 (tree exp, rtx target
*** 9241,9247 ****
  	addr_space_t as = TYPE_ADDR_SPACE (TREE_TYPE (exp));
  	struct mem_address addr;
  	enum insn_code icode;
! 	int align;
  
  	get_address_description (exp, &addr);
  	op0 = addr_for_mem_ref (&addr, as, true);
--- 9262,9268 ----
  	addr_space_t as = TYPE_ADDR_SPACE (TREE_TYPE (exp));
  	struct mem_address addr;
  	enum insn_code icode;
! 	unsigned int align;
  
  	get_address_description (exp, &addr);
  	op0 = addr_for_mem_ref (&addr, as, true);
*************** expand_expr_real_1 (tree exp, rtx target
*** 9249,9257 ****
  	temp = gen_rtx_MEM (mode, op0);
  	set_mem_attributes (temp, exp, 0);
  	set_mem_addr_space (temp, as);
! 	align = MAX (TYPE_ALIGN (TREE_TYPE (exp)), get_object_alignment (exp));
  	if (mode != BLKmode
! 	    && (unsigned) align < GET_MODE_ALIGNMENT (mode)
  	    /* If the target does not have special handling for unaligned
  	       loads of mode then it can use regular moves for them.  */
  	    && ((icode = optab_handler (movmisalign_optab, mode))
--- 9270,9278 ----
  	temp = gen_rtx_MEM (mode, op0);
  	set_mem_attributes (temp, exp, 0);
  	set_mem_addr_space (temp, as);
! 	align = get_object_or_type_alignment (exp);
  	if (mode != BLKmode
! 	    && align < GET_MODE_ALIGNMENT (mode)
  	    /* If the target does not have special handling for unaligned
  	       loads of mode then it can use regular moves for them.  */
  	    && ((icode = optab_handler (movmisalign_optab, mode))
*************** expand_expr_real_1 (tree exp, rtx target
*** 9278,9284 ****
  	tree base = TREE_OPERAND (exp, 0);
  	gimple def_stmt;
  	enum insn_code icode;
! 	int align;
  	/* Handle expansion of non-aliased memory with non-BLKmode.  That
  	   might end up in a register.  */
  	if (TREE_CODE (base) == ADDR_EXPR)
--- 9299,9305 ----
  	tree base = TREE_OPERAND (exp, 0);
  	gimple def_stmt;
  	enum insn_code icode;
! 	unsigned align;
  	/* Handle expansion of non-aliased memory with non-BLKmode.  That
  	   might end up in a register.  */
  	if (TREE_CODE (base) == ADDR_EXPR)
*************** expand_expr_real_1 (tree exp, rtx target
*** 9329,9335 ****
  			   gimple_assign_rhs1 (def_stmt), mask);
  	    TREE_OPERAND (exp, 0) = base;
  	  }
! 	align = MAX (TYPE_ALIGN (TREE_TYPE (exp)), get_object_alignment (exp));
  	op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_SUM);
  	op0 = memory_address_addr_space (address_mode, op0, as);
  	if (!integer_zerop (TREE_OPERAND (exp, 1)))
--- 9350,9356 ----
  			   gimple_assign_rhs1 (def_stmt), mask);
  	    TREE_OPERAND (exp, 0) = base;
  	  }
! 	align = get_object_or_type_alignment (exp);
  	op0 = expand_expr (base, NULL_RTX, VOIDmode, EXPAND_SUM);
  	op0 = memory_address_addr_space (address_mode, op0, as);
  	if (!integer_zerop (TREE_OPERAND (exp, 1)))
*************** expand_expr_real_1 (tree exp, rtx target
*** 9345,9351 ****
  	if (TREE_THIS_VOLATILE (exp))
  	  MEM_VOLATILE_P (temp) = 1;
  	if (mode != BLKmode
! 	    && (unsigned) align < GET_MODE_ALIGNMENT (mode)
  	    /* If the target does not have special handling for unaligned
  	       loads of mode then it can use regular moves for them.  */
  	    && ((icode = optab_handler (movmisalign_optab, mode))
--- 9366,9372 ----
  	if (TREE_THIS_VOLATILE (exp))
  	  MEM_VOLATILE_P (temp) = 1;
  	if (mode != BLKmode
! 	    && align < GET_MODE_ALIGNMENT (mode)
  	    /* If the target does not have special handling for unaligned
  	       loads of mode then it can use regular moves for them.  */
  	    && ((icode = optab_handler (movmisalign_optab, mode))


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]