Fix ARM constant-pool layout calculations under -falign-labels

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.


Thanks,
Roland


2012-08-01  Roland McGrath  <mcgrathr@google.com>

	* config/arm/arm.c (get_label_padding): Use align_labels as minimum.

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