On Tue, Oct 25, 2011 at 2:22 PM, Tom de Vries <tom_devr...@mentor.com> wrote: > On 09/24/2011 01:42 PM, Richard Guenther wrote: >> On Sat, Sep 24, 2011 at 11:40 AM, Jakub Jelinek <ja...@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. 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. + /* 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 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). + 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. + if (pi->align >= align) + break; + pi->align = align; and then set pi->misalign to zero here. 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. + 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? /* 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. The * gcc.c-torture/unsorted/HIcmp.c: Fix unaligned pointer. * gcc.c-torture/unsorted/HIset.c: Same. * gcc.c-torture/unsorted/SIcmp.c: Same. * gcc.c-torture/unsorted/SIset.c: Same. * gcc.c-torture/unsorted/SFset.c: Same. * gcc.c-torture/unsorted/UHIcmp.c: Same. * gcc.c-torture/unsorted/USIcmp.c: Same. * gcc.c-torture/unsorted/DFcmp.c: Same. changes are ok with s/sizeof/alignof/ Thanks, Richard. > Thanks, > - Tom > >> Richard. >> >>> Jakub >>> > > 2011-10-25 Tom de Vries <t...@codesourcery.com> > > PR target/43814 > * tree-ssa-ccp.c (get_align_value): New function, factored out of > get_value_from_alignment. > (get_value_from_alignment): Use get_align_value. > (get_value_for_expr): Use get_align_value to handle alignment of > pointers with ptr_info->align set. > (deduce_alignment_from_dereferences): New function. > (do_ssa_ccp): Calculate post-dominance info and call > deduce_alignment_from_dereferences. > > * gcc/testsuite/gcc.target/arm/pr43814-2.c: New test. > * gcc.c-torture/unsorted/HIcmp.c: Fix unaligned pointer. > * gcc.c-torture/unsorted/HIset.c: Same. > * gcc.c-torture/unsorted/SIcmp.c: Same. > * gcc.c-torture/unsorted/SIset.c: Same. > * gcc.c-torture/unsorted/SFset.c: Same. > * gcc.c-torture/unsorted/UHIcmp.c: Same. > * gcc.c-torture/unsorted/USIcmp.c: Same. > * gcc.c-torture/unsorted/DFcmp.c: Same. >