| Message ID | 201112071532.59754.ebotcazou@adacore.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 7, 2011 at 3:32 PM, Eric Botcazou <ebotcazou@adacore.com> wrote: >> You are basically (but non-optimally) locally re-implementing >> what expr.c:get_object_or_type_alignment does, for the >> bare MEM_REF case (you don't consider offsets that >> do not change the alignment, nor alignment information >> present on the SSA name). > > Adjusted version attached, regression-tested on SPARC/Solaris. I'll do a full > testing cycle if it is OK for mainline. Yes, the patch is ok for mainline. > get_object_or_type_alignment doesn't exist on the 4.6 branch, the expanders for > MEM_REF and TARGET_MEM_REF do: > > MAX (TYPE_ALIGN (TREE_TYPE (exp)), > get_object_alignment (exp, BIGGEST_ALIGNMENT))) > > instead, so I presume I should do the same for them in tree_non_aligned_mem_p? Yeah, you can do that. Thanks, Richard. > > 2011-12-07 Eric Botcazou <ebotcazou@adacore.com> > > PR tree-optimization/51315 > * tree.h (get_object_or_type_alignment): Declare. > * expr.c (get_object_or_type_alignment): Move to... > * builtins.c (get_object_or_type_alignment): ...here. Add assertion. > * tree-sra.c (tree_non_mode_aligned_mem_p): Rename to... > (tree_non_aligned_mem_p): ...this. Add ALIGN parameter. Look into > MEM_REFs and use get_object_or_type_alignment for them. > (build_accesses_from_assign): Adjust for above change. > (access_precludes_ipa_sra_p): Likewise. > > > > -- > Eric Botcazou
Eric Botcazou <ebotcazou@adacore.com> writes: >> You are basically (but non-optimally) locally re-implementing >> what expr.c:get_object_or_type_alignment does, for the >> bare MEM_REF case (you don't consider offsets that >> do not change the alignment, nor alignment information >> present on the SSA name). > > Adjusted version attached, regression-tested on SPARC/Solaris. I'll do a full > testing cycle if it is OK for mainline. > > get_object_or_type_alignment doesn't exist on the 4.6 branch, the expanders for > MEM_REF and TARGET_MEM_REF do: > > MAX (TYPE_ALIGN (TREE_TYPE (exp)), > get_object_alignment (exp, BIGGEST_ALIGNMENT))) > > instead, so I presume I should do the same for them in tree_non_aligned_mem_p? > > > 2011-12-07 Eric Botcazou <ebotcazou@adacore.com> > > PR tree-optimization/51315 > * tree.h (get_object_or_type_alignment): Declare. > * expr.c (get_object_or_type_alignment): Move to... > * builtins.c (get_object_or_type_alignment): ...here. Add assertion. > * tree-sra.c (tree_non_mode_aligned_mem_p): Rename to... > (tree_non_aligned_mem_p): ...this. Add ALIGN parameter. Look into > MEM_REFs and use get_object_or_type_alignment for them. > (build_accesses_from_assign): Adjust for above change. > (access_precludes_ipa_sra_p): Likewise. Sorry for the late notice, but this regressed memcpy-1.c for n32 and n64 on mips64-linux-gnu. I realise memcpy-1.c isn't viewed as being a proper SRA test, but in this case I think it really is showing a genuine problem. We have: struct a {int a,b,c;} a; int test(struct a a) { struct a nasty_local; __builtin_memcpy (&nasty_local,&a, sizeof(a)); return nasty_local.a; } We apply LOCAL_ALIGNMENT to nasty_local during estimated_stack_frame_size, so we have a VAR_DECL (nasty_local) with 64-bit alignment and a PARM_DECL (a) with 32-bit alignment. This fails the condition: if (STRICT_ALIGNMENT && tree_non_aligned_mem_p (rhs, get_object_alignment (lhs))) lacc->grp_unscalarizable_region = 1; because LHS has 64-bit alignment but RHS has 32-bit alignment. Richard
On Mon, Jan 2, 2012 at 8:00 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Eric Botcazou <ebotcazou@adacore.com> writes: >>> You are basically (but non-optimally) locally re-implementing >>> what expr.c:get_object_or_type_alignment does, for the >>> bare MEM_REF case (you don't consider offsets that >>> do not change the alignment, nor alignment information >>> present on the SSA name). >> >> Adjusted version attached, regression-tested on SPARC/Solaris. I'll do a full >> testing cycle if it is OK for mainline. >> >> get_object_or_type_alignment doesn't exist on the 4.6 branch, the expanders for >> MEM_REF and TARGET_MEM_REF do: >> >> MAX (TYPE_ALIGN (TREE_TYPE (exp)), >> get_object_alignment (exp, BIGGEST_ALIGNMENT))) >> >> instead, so I presume I should do the same for them in tree_non_aligned_mem_p? >> >> >> 2011-12-07 Eric Botcazou <ebotcazou@adacore.com> >> >> PR tree-optimization/51315 >> * tree.h (get_object_or_type_alignment): Declare. >> * expr.c (get_object_or_type_alignment): Move to... >> * builtins.c (get_object_or_type_alignment): ...here. Add assertion. >> * tree-sra.c (tree_non_mode_aligned_mem_p): Rename to... >> (tree_non_aligned_mem_p): ...this. Add ALIGN parameter. Look into >> MEM_REFs and use get_object_or_type_alignment for them. >> (build_accesses_from_assign): Adjust for above change. >> (access_precludes_ipa_sra_p): Likewise. > > Sorry for the late notice, but this regressed memcpy-1.c for n32 and n64 > on mips64-linux-gnu. I realise memcpy-1.c isn't viewed as being a proper > SRA test, but in this case I think it really is showing a genuine problem. > We have: > > struct a {int a,b,c;} a; > int test(struct a a) > { > struct a nasty_local; > __builtin_memcpy (&nasty_local,&a, sizeof(a)); > return nasty_local.a; > } > > We apply LOCAL_ALIGNMENT to nasty_local during estimated_stack_frame_size, > so we have a VAR_DECL (nasty_local) with 64-bit alignment and a PARM_DECL > (a) with 32-bit alignment. This fails the condition: > > if (STRICT_ALIGNMENT > && tree_non_aligned_mem_p (rhs, get_object_alignment (lhs))) > lacc->grp_unscalarizable_region = 1; > > because LHS has 64-bit alignment but RHS has 32-bit alignment. Shouldn't we go back and unconditionally return false if lhs and (or?) rhs are BLKmode? I suppose they are, in this case. Or should we truncate the alignment from get_object_alignment to TYPE_ALIGN if it is bigger than that to avoid this situation? I suppose Eric will know which case the expander would handle correctly. Richard. > Richard
Index: tree.h =================================================================== --- tree.h (revision 181993) +++ tree.h (working copy) @@ -5463,6 +5463,7 @@ extern rtx builtin_memset_read_str (void extern bool is_builtin_fn (tree); extern unsigned int get_object_alignment_1 (tree, unsigned HOST_WIDE_INT *); extern unsigned int get_object_alignment (tree); +extern unsigned int get_object_or_type_alignment (tree); extern unsigned int get_pointer_alignment (tree); extern tree fold_call_stmt (gimple, bool); extern tree gimple_fold_builtin_snprintf_chk (gimple, tree, enum built_in_function); Index: builtins.c =================================================================== --- builtins.c (revision 181993) +++ builtins.c (working copy) @@ -452,6 +452,31 @@ get_object_alignment (tree exp) return align; } +/* Return the alignment of object EXP, also considering its type when we do + not know of explicit misalignment. Only handle MEM_REF and TARGET_MEM_REF. + + ??? 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 this alignment when we cannot compute a misalignment. */ + +unsigned int +get_object_or_type_alignment (tree exp) +{ + unsigned HOST_WIDE_INT misalign; + unsigned int align = get_object_alignment_1 (exp, &misalign); + + gcc_assert (TREE_CODE (exp) == MEM_REF || TREE_CODE (exp) == TARGET_MEM_REF); + + if (misalign != 0) + align = (misalign & -misalign); + else + align = MAX (TYPE_ALIGN (TREE_TYPE (exp)), align); + + return align; +} + /* Return the alignment in bits of EXP, a pointer valued expression. The alignment returned is, by default, the alignment of the thing that EXP points to. If it is not a POINTER_TYPE, 0 is returned. Index: expr.c =================================================================== --- expr.c (revision 181993) +++ expr.c (working copy) @@ -4544,27 +4544,6 @@ get_bit_range (unsigned HOST_WIDE_INT *b } } -/* 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. */ Index: tree-sra.c =================================================================== --- tree-sra.c (revision 181993) +++ tree-sra.c (working copy) @@ -1067,26 +1067,29 @@ disqualify_ops_if_throwing_stmt (gimple return false; } -/* Return true iff type of EXP is not sufficiently aligned. */ +/* Return true if EXP is a memory reference less aligned than ALIGN. This is + invoked only on strict-alignment targets. */ static bool -tree_non_mode_aligned_mem_p (tree exp) +tree_non_aligned_mem_p (tree exp, unsigned int align) { - enum machine_mode mode = TYPE_MODE (TREE_TYPE (exp)); - unsigned int align; + unsigned int exp_align; if (TREE_CODE (exp) == VIEW_CONVERT_EXPR) exp = TREE_OPERAND (exp, 0); - if (TREE_CODE (exp) == SSA_NAME - || TREE_CODE (exp) == MEM_REF - || mode == BLKmode - || is_gimple_min_invariant (exp) - || !STRICT_ALIGNMENT) + if (TREE_CODE (exp) == SSA_NAME || is_gimple_min_invariant (exp)) return false; - align = get_object_alignment (exp); - if (GET_MODE_ALIGNMENT (mode) > align) + /* get_object_alignment will fall back to BITS_PER_UNIT if it cannot + compute an explicit alignment. Pretend that dereferenced pointers + are always aligned on strict-alignment targets. */ + if (TREE_CODE (exp) == MEM_REF || TREE_CODE (exp) == TARGET_MEM_REF) + exp_align = get_object_or_type_alignment (exp); + else + exp_align = get_object_alignment (exp); + + if (exp_align < align) return true; return false; @@ -1120,7 +1123,9 @@ build_accesses_from_assign (gimple stmt) if (lacc) { lacc->grp_assignment_write = 1; - lacc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (rhs); + if (STRICT_ALIGNMENT + && tree_non_aligned_mem_p (rhs, get_object_alignment (lhs))) + lacc->grp_unscalarizable_region = 1; } if (racc) @@ -1129,7 +1134,9 @@ build_accesses_from_assign (gimple stmt) if (should_scalarize_away_bitmap && !gimple_has_volatile_ops (stmt) && !is_gimple_reg_type (racc->type)) bitmap_set_bit (should_scalarize_away_bitmap, DECL_UID (racc->base)); - racc->grp_unscalarizable_region |= tree_non_mode_aligned_mem_p (lhs); + if (STRICT_ALIGNMENT + && tree_non_aligned_mem_p (lhs, get_object_alignment (rhs))) + racc->grp_unscalarizable_region = 1; } if (lacc && racc @@ -3705,7 +3712,8 @@ access_precludes_ipa_sra_p (struct acces || gimple_code (access->stmt) == GIMPLE_ASM)) return true; - if (tree_non_mode_aligned_mem_p (access->expr)) + if (STRICT_ALIGNMENT + && tree_non_aligned_mem_p (access->expr, TYPE_ALIGN (access->type))) return true; return false;