Message ID | CAB=4xhqNVD5FvFdrN8kqrvqngLifAD320bg5jt0aw7Opotbtxw@mail.gmail.com |
---|---|
State | New |
Headers | show |
On Wed, 1 Aug 2012 14:43:33 -0700 Roland McGrath <mcgrathr@google.com> wrote: > Using e.g. -falign-labels=16 on ARM can confuse the constant-pool > layout code such that it places pool entries too far away from their > referring instructions. This change seems to fix it. > > I don't have a small test case, only a large one, which I haven't > actually tried to get to reproduce on any vanilla ARM target. But > the logic of the change seems straightforward and sound. FWIW, I've hit this issue in the past, and used a patch as follows to fix it: @@ -12015,7 +12025,10 @@ create_fix_barrier (Mfix *fix, HOST_WIDE gcc_assert (GET_CODE (from) != BARRIER); /* Count the length of this insn. */ - count += get_attr_length (from); + if (LABEL_P (from) && (align_jumps > 0 || align_loops > 0)) + count += MAX (align_jumps, align_loops); + else + count += get_attr_length (from); /* If there is a jump table, add its length. */ tmp = is_jump_table (from); @@ -12435,6 +12448,8 @@ arm_reorg (void) insn = table; } } + else if (LABEL_P (insn) && (align_jumps > 0 || align_loops > 0)) + address += MAX (align_jumps, align_loops); } fix = minipool_fix_head; Not sure if that's equivalent, but you might want to check -falign-jumps too while you're at it. Cheers, Julian
On Thu, 2 Aug 2012 09:57:40 +0100 Julian Brown <julian@codesourcery.com> wrote: > On Wed, 1 Aug 2012 14:43:33 -0700 > Roland McGrath <mcgrathr@google.com> wrote: > > > Using e.g. -falign-labels=16 on ARM can confuse the constant-pool > > layout code such that it places pool entries too far away from their > > referring instructions. This change seems to fix it. > > > > I don't have a small test case, only a large one, which I haven't > > actually tried to get to reproduce on any vanilla ARM target. But > > the logic of the change seems straightforward and sound. > > FWIW, I've hit this issue in the past, and used a patch as follows to > fix it: > > @@ -12015,7 +12025,10 @@ create_fix_barrier (Mfix *fix, HOST_WIDE > gcc_assert (GET_CODE (from) != BARRIER); > > /* Count the length of this insn. */ > - count += get_attr_length (from); > + if (LABEL_P (from) && (align_jumps > 0 || align_loops > 0)) > + count += MAX (align_jumps, align_loops); > + else > + count += get_attr_length (from); > > /* If there is a jump table, add its length. */ > tmp = is_jump_table (from); > @@ -12435,6 +12448,8 @@ arm_reorg (void) > insn = table; > } > } > + else if (LABEL_P (insn) && (align_jumps > 0 || align_loops > > 0)) > + address += MAX (align_jumps, align_loops); > } > > fix = minipool_fix_head; > > Not sure if that's equivalent, but you might want to check > -falign-jumps too while you're at it. (...and -falign-loops. Whoops, misread :-)).
On Thu, Aug 2, 2012 at 1:57 AM, Julian Brown <julian@codesourcery.com> wrote: > FWIW, I've hit this issue in the past, and used a patch as follows to > fix it: > > @@ -12015,7 +12025,10 @@ create_fix_barrier (Mfix *fix, HOST_WIDE > gcc_assert (GET_CODE (from) != BARRIER); > > /* Count the length of this insn. */ > - count += get_attr_length (from); > + if (LABEL_P (from) && (align_jumps > 0 || align_loops > 0)) > + count += MAX (align_jumps, align_loops); > + else > + count += get_attr_length (from); > That looks like it's from a version of arm.c before get_label_padding was added. The existing trunk code uses get_label_padding at that spot, and my change was to get_label_padding. > Not sure if that's equivalent, but you might want to check > [-falign-loops] too while you're at it. That seems like it could overestimate, if the -falign-loops value is larger than the -falign-labels value. Not every label is for a loop. Thanks, Roland
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c index 701ab4c..9bdb52c 100644 --- a/gcc/config/arm/arm.c +++ b/gcc/config/arm/arm.c @@ -12384,6 +12384,7 @@ get_label_padding (rtx label) HOST_WIDE_INT align, min_insn_size; align = 1 << label_to_alignment (label); + align = MAX (align, align_labels); min_insn_size = TARGET_THUMB ? 2 : 4; return align > min_insn_size ? align - min_insn_size : 0; }