[PR43814] Assume function arguments of pointer type are aligned.

On 26/10/11 12:19, Richard Guenther wrote:
> On Tue, Oct 25, 2011 at 2:22 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
>> On 09/24/2011 01:42 PM, Richard Guenther wrote:
>>> On Sat, Sep 24, 2011 at 11:40 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>>> On Sat, Sep 24, 2011 at 11:31:25AM +0200, Richard Guenther wrote:
>>>>> In the end I'd probably say the patch is ok without the option (thus
>>>>> turned on by default), but if LC_GLOBAL_LOCALE is part of the
>>>>> glibc ABI then we clearly can't do this.
>>>>
>>>> Yes, LC_GLOBAL_LOCALE is part of glibc ABI.  I guess we could only assume
>>>> the alignment if the pointer is actually dereferenced on the statement
>>>> that checks the ABI or in some stmt that dominates the spot where you want
>>>> to check the alignment.  It is IMHO quite common to pass arbitrary values
>>>> in pointer types, then cast them back or just compare.
>>>
>>> Yeah (even if technically invoking undefined behavior in C).  Checking if
>>> there is a dereference post-dominating function entry with sth like
>>>
>>>   FOR_EACH_IMM_USE_STMT (... ptr ...)
>>>      if (stmt_post_dominates_entry && contains derefrence of ptr)
>>>        alignment = TYPE_ALIGN (...);
>>>
>>> and otherwise not assuming anything about parameter alignment might work.
>>> Be careful to check the alignment of the dereference though,
>>>
>>> typedef int int_unaligned __attribute__((aligned(1)));
>>> int foo (int *p)
>>> {
>>>   int_unaligned *q = p;
>>>   return *q;
>>> }
>>>
>>> will be MEM[p] but with (well, hopefully ;)) TYPE_ALIGN of TREE_TYPE (MEM[p])
>>> being 1.  And yes, you'd have to look into handled-components as well.  I guess
>>> you'll face similar problems as we do with tree-sra.c
>>> tree_non_mode_aligned_mem_p
>>> (you need to assume eventually misaligned accesses the same way expansion
>>> does for the dereference, otherwise you'll run into issues on
>>> strict-align targets).
>>>
>>> As that de-refrence thing doesn't really fit the CCP propagation you
>>> won't be able
>>> to handle
>>>
>>> int foo (int *p)
>>> {
>>>   int *q = (char *)p + 3;
>>>   return *q;
>>> }
>>>
>>> and assume q is aligned (and p is misaligned by 1).
>>>
>>> That is, if the definition of a pointer is post-dominated by a derefrence
>>> we could assume proper alignment for that pointer (as opposed to just
>>> special-casing its default definition).  Would be certainly interesting to
>>> see what kind of fallout we would get from that ;)
>>>
>>
>> I gave this a try in deduce_alignment_from_dereferences.
>>
>> The fall-out I got from this were unaligned dereferenced pointers in
>> gcc.c-torture/unsorted/*{cmp,set}.c.
>>
>> Bootstrapped and reg-tested on x86_64. Build and reg-tested on MIPS and ARM.
>>
>> Ok for trunk?
> 
> Can you not do the get_value_from_alignment split (it doesn't look
> necessary to me) and drop the
> 
> @@ -541,10 +550,18 @@ get_value_for_expr (tree expr, bool for_
>    if (TREE_CODE (expr) == SSA_NAME)
>      {
>        val = *get_value (expr);
> -      if (for_bits_p
> -         && val.lattice_val == CONSTANT
> +      if (!for_bits_p)
> +       return val;
> +
> +      if (val.lattice_val == CONSTANT
>           && TREE_CODE (val.value) == ADDR_EXPR)
>         val = get_value_from_alignment (val.value);
> +      else if (val.lattice_val == VARYING
> +              && SSA_NAME_PTR_INFO (expr) != NULL
> +              && SSA_NAME_PTR_INFO (expr)->align > 1
> +              && SSA_NAME_PTR_INFO (expr)->misalign == 0)
> +       val = get_align_value (SSA_NAME_PTR_INFO (expr)->align * BITS_PER_UNIT,
> +                              TREE_TYPE (expr), 0);
>      }
> 
> hunk?  I'm not sure why it is necessary at all - CCP is the only pass
> computing alignment, so it should simply re-compute the info?
> 
> Anyway, it looks unrelated to the purpose of the patch in general.
> 

I dropped the code in get_value_for_expr, but kept the factoring of
get_align_value out of get_value_from_alignment . This remains necessary in the
new patch since get_align_value is still called from 2 sites:
get_value_from_alignment and deduce_alignment_from_dereference.

> The error reporting in deduce_alignment_from_dereferences is bogus,
> the programs are undefined only at runtime, so you can at most
> issue a warning.
> 

Introduced -Wunaligned-pointer-deref.

> +  /* Needs to be the successor of entry, for CDI_POST_DOMINATORS.  */
> +  entry = single_succ (ENTRY_BLOCK_PTR);
> +
> +  FOR_EACH_BB (bb)
> +    {
> +      gimple_stmt_iterator i;
> +
> +      if (!dominated_by_p (CDI_POST_DOMINATORS, entry, bb))
> +       continue;
> 
> if you only consider post-dominators of the entry block then just walk
> them directly (first_dom_son / next_dom_son).
> 
> +             align = TYPE_ALIGN (TREE_TYPE (memref)) / BITS_PER_UNIT;
> +             if (align == 1)
> +               continue;
> 

I integrated the walking of post-dominators into the bb walk in ccp_initialize.

> I think you want to match what expand thinks of the alignment of this
> memory reference, not just what TYPE_ALIGN says (and yes, that
> needs to be split out somehow, SRA would need this as well).
> 

I'm now using get_object_or_type_alignment for MEM_REF.

> +         while (TREE_CODE (ptr) == SSA_NAME)
> +           {
> +             pi = get_ptr_info (ptr);
> +             if (pi->misalign != 0)
> +               {
> +                 error ("misaligned pointer dereferenced");
> +                 break;
> +               }
> 
> simply looking at pi->misalign is wrong.  pi->align may be bigger
> than the align that you computed above, so pi->misalign % align != 0
> would be the right check.
> 

This is my slightly more complex misalign check now:
...
+      if ((pi->align < align && misalign % pi->align != pi->misalign)
+	  || (pi->align >= align && pi->misalign % align != misalign))
...

> +             if (pi->align >= align)
> +               break;
> +             pi->align = align;
> 
> and then set pi->misalign to zero here.

The patch no longer sets pointer info directly, as per your next comment. The
align/misalign is now stored in const_val:
...
+      val = get_align_value (align * BITS_PER_UNIT, TREE_TYPE (ptr),
+			     misalign * BITS_PER_UNIT);
+      const_val[SSA_NAME_VERSION (ptr)] = val;
...

> But I would initialize the
> CCP lattice with this, not set SSA_NAME_PTR_INFO, from
> ccp_initialize, that already walks over all blocks and statements.
> 

Done.

> +             if (gimple_assign_rhs_code (def) == POINTER_PLUS_EXPR)
> +               {
> +                 offset = gimple_assign_rhs2 (def);
> +                 if (!host_integerp (offset, 0)
> +                     || tree_low_cst (offset, 0) % align != 0)
> +                   break;
> +
> +                 ptr = gimple_assign_rhs1 (def);
> 
> properly tracking explicit misalignment would be useful here, but I
> see you are posting a proof-of-concept?

The new patch attempts to keep track of explicit misalignment.

> 
>  /* Main entry point for SSA Conditional Constant Propagation.  */
> 
>  static unsigned int
>  do_ssa_ccp (void)
>  {
> +  calculate_dominance_info (CDI_POST_DOMINATORS);
> +  deduce_alignment_from_dereferences ();
> 
> you need to free post-dom info again.
> 

Done.

Bootstrapped and reg-tested on x86_64, build and reg-tested on ARM.

Ok for next stage1?

Thanks,
- Tom

2011-12-08  Tom de Vries <tom@codesourcery.com>

	PR target/43814
	* expr.c (get_object_or_type_alignment): Remove static.
	* expr.h (get_object_or_type_alignment): Declare.
	* common.opt (Wunaligned-pointer-deref): New option.
	* tree-ssa-ccp.c (expr.h): Include.
	(alignment_to_address_transition_p): New function.
	(valid_lattice_transition): Allow transition from CONSTANT CONST_INT to
	CONSTANT ADDR_EXPR.
	(set_lattice_value): Likewise.  Ignore transitions from CONSTANT to
	UNDEFINED.
	(get_align_value): New function, factored out of
	get_value_from_alignment.
	(get_value_from_alignment): Use get_align_value.
	(deduce_alignment_from_dereference): New function.
	(ccp_initialize): call deduce_alignment_from_dereference for statements
	in block that are post-dominated by the entry block.
	(do_ssa_ccp): Calculate and free post-dominance info.

	* gcc.target/arm/pr43814-2.c: New test.
	* gcc.dg/misalign.c: Same.
	* gcc.dg/misalign2.c: Same.
	* gcc.dg/analyze-deref.c: Same.

[PR43814] Assume function arguments of pointer type are aligned.

Commit Message

Comments

Patch