[RFC PATCH] .bundle_align_mode

[Roland McGrath <mcgrathr at google dot com>]

This patch is a bit of a straw-man to get some advice on how best to
implement the feature.  Rest assured a real patch will have log
entries, cleaner code, more comments, testsuite coverage, etc.  So
don't bother yet with telling me to do those obvious things.

The as.texinfo patch should explain what functionality I'm trying to
achieve.  (There's an additional variant of the feature I'd like to
figure out how to implement, but I won't go into that before I get
feedback on what I've done so far.)  I'm entirely open to changing
the names and detailed semantics of the directives and so forth to
comport with the consensus among gas maintainers, of course
constrained to details that actually address the needs I'm after.

I'm looking for feedback from folks who grok the gas internals well,
both on the general implementation approach and specifically on the
appropriate interface for backend hooks (md_max_size in this patch).
(After that I'll of course also want the backend maintainers' advice
on the optimal implementation of the hooks in each backend I'm
tackling, which in the near term will be only x86 and ARM/Thumb.)

This is to meet the requirements of particular targets (various
Native Client ABIs, of which there will be more in the future), but
I think it's more maintainable and less work to implement it as a
generic (optional) feature.  Though it makes the feature always
available for any target, what I've done here has no effect
whatsoever when you don't use the .bundle_align_mode directive to
switch it on.

I'm aware that the basic approach is not good for assembler
performance, essentially creating a frag or two for every
instruction.  I'm not really concerned about that problem.
(Assembly-time performance is just not a practical bottleneck.)  But
if you can suggest different approaches that are not much harder to
implement but scale better, of course I'd be glad to see those.

I'm looking for a plan that is not desperately difficult or invasive
to implement, minimizes the new machine-dependent code required, and
that I can understand.  (I don't grok gas internals especially well
despite 20+ years of occasionally fiddling with them, but I'd be
happy to be taught.)  If there is a more clever method that involves
even less machine-dependent code than this, by being closely tied in
with relaxation or something, then I'd be very glad to find it.

The approach I've taken here is pessimistic in relaxation cases.
For example, if a jump instruction would appear two bytes before an
instruction bundle alignment boundary, then it will always be bumped
to that boundary on the assumption that it may need more than two
bytes to encode.  If relaxation in fact delivers a two-byte
encoding, then this was unnecessary but we can't undo it.  I think
this suboptimality is an acceptable trade-off for making the
implementation tractable.  But I'd be thrilled to find a tractable
plan that is more optimal in even some subset of cases.


Thanks,
Roland


diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
index 02a63a6..7634d74 100644
--- a/gas/config/tc-arm.c
+++ b/gas/config/tc-arm.c
@@ -1,7 +1,5 @@
 /* tc-arm.c -- Assemble for the ARM
-   Copyright 1994, 1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003,
-   2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
-   Free Software Foundation, Inc.
+   Copyright 1994-2012 Free Software Foundation, Inc.
    Contributed by Richard Earnshaw (rwe@pegasus.esprit.ec.org)
 	Modified by David Taylor (dtaylor@armltd.co.uk)
 	Cirrus coprocessor mods by Aldy Hernandez (aldyh@redhat.com)
@@ -18998,6 +18996,12 @@ md_chars_to_number (char * buf, int n)
 
 /* MD interface: Sections.  */
 
+int
+arm_max_size (fragS *fragp ATTRIBUTE_UNUSED)
+{
+  return 4;
+}
+
 /* Estimate the size of a frag before relaxing.  Assume everything fits in
    2 bytes.  */
 
diff --git a/gas/config/tc-arm.h b/gas/config/tc-arm.h
index 2916ae1..a7ebb40 100644
--- a/gas/config/tc-arm.h
+++ b/gas/config/tc-arm.h
@@ -1,6 +1,5 @@
 /* This file is tc-arm.h
-   Copyright 1994, 1995, 1996, 1997, 1998, 1999, 2000, 2001, 2002, 2003,
-   2004, 2005, 2006, 2007, 2008, 2009  Free Software Foundation, Inc.
+   Copyright 1994-2012 Free Software Foundation, Inc.
    Contributed by Richard Earnshaw (rwe@pegasus.esprit.ec.org)
 	Modified by David Taylor (dtaylor@armltd.co.uk)
 
@@ -81,6 +80,9 @@ struct fix;
 
 #define TC_FORCE_RELOCATION(FIX) arm_force_relocation (FIX)
 
+#define md_max_size(fragp) arm_max_size(fragp)
+extern int arm_max_size (struct frag *);
+
 #define md_relax_frag(segment, fragp, stretch) \
   arm_relax_frag (segment, fragp, stretch)
 extern int arm_relax_frag (asection *, struct frag *, long);
diff --git a/gas/config/tc-i386.c b/gas/config/tc-i386.c
index 97cb68e..3bbb867 100644
--- a/gas/config/tc-i386.c
+++ b/gas/config/tc-i386.c
@@ -1,8 +1,5 @@
 /* tc-i386.c -- Assemble code for the Intel 80386
-   Copyright 1989, 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999,
-   2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011,
-   2012
-   Free Software Foundation, Inc.
+   Copyright 1989,1991-2012 Free Software Foundation, Inc.
 
    This file is part of GAS, the GNU Assembler.
 
@@ -7715,6 +7712,15 @@ i386_att_operand (char *operand_string)
   return 1;			/* Normal return.  */
 }
 
+/* Called in between md_assemble calls, to calculate the maximum size an
+   rs_machine_dependent frag will have after relaxation.  */
+int
+i386_max_size (fragS *frag)
+{
+  /* The only relaxable frags are for jumps, and these add at most 5 bytes.  */
+  return frag->fr_fix + 5;
+}
+
 /* md_estimate_size_before_relax()
 
    Called just before relax() for rs_machine_dependent frags.  The x86
diff --git a/gas/config/tc-i386.h b/gas/config/tc-i386.h
index 688c69a..846a5cc 100644
--- a/gas/config/tc-i386.h
+++ b/gas/config/tc-i386.h
@@ -1,7 +1,5 @@
 /* tc-i386.h -- Header file for tc-i386.c
-   Copyright 1989, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000,
-   2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2012
-   Free Software Foundation, Inc.
+   Copyright 1989,1992-2012 Free Software Foundation, Inc.
 
    This file is part of GAS, the GNU Assembler.
 
@@ -215,6 +213,9 @@ if (fragP->fr_type == rs_align_code) 					\
 void i386_print_statistics (FILE *);
 #define tc_print_statistics i386_print_statistics
 
+extern int i386_max_size (fragS *);
+#define md_max_size i386_max_size
+
 #define md_number_to_chars number_to_chars_littleendian
 
 enum processor_type
diff --git a/gas/doc/as.texinfo b/gas/doc/as.texinfo
index 7a41f02..fd8f360 100644
--- a/gas/doc/as.texinfo
+++ b/gas/doc/as.texinfo
@@ -1,7 +1,5 @@
 \input texinfo @c                               -*-Texinfo-*-
-@c  Copyright 1991, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999, 2000,
-@c  2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011
-@c  Free Software Foundation, Inc.
+@c Copyright (C) 1991-2012 Free Software Foundation, Inc.
 @c UPDATE!!  On future updates--
 @c   (1)   check for new machine-dep cmdline options in
 @c         md_parse_option definitions in config/tc-*.c
@@ -3970,6 +3968,7 @@ Some machine configurations provide additional directives.
 * Ascii::                       @code{.ascii "@var{string}"}@dots{}
 * Asciz::                       @code{.asciz "@var{string}"}@dots{}
 * Balign::                      @code{.balign @var{abs-expr} , @var{abs-expr}}
+* Bundle directives::           @code{.bundle_align_mode @var{abs-expr}}, @code{.bundle_lock}, @code{.bundle_unlock}
 * Byte::                        @code{.byte @var{expressions}}
 * CFI directives::		@code{.cfi_startproc [simple]}, @code{.cfi_endproc}, etc.
 * Comm::                        @code{.comm @var{symbol} , @var{length} }
@@ -4292,6 +4291,49 @@ filled in with the value 0x368d (the exact placement of the bytes depends upon
 the endianness of the processor).  If it skips 1 or 3 bytes, the fill value is
 undefined.
 
+@node Bundle directives
+@section @code{.bundle_align_mode @var{abs-expr}}
+@cindex @code{bundle_align_mode} directive
+@cindex bundle
+@cindex instruction bundle
+@cindex aligned instruction bundle
+@code{.bundle_align_mode} enables or disables @defn{aligned instruction bundle}
+mode.  In this mode, sequences of adjacent instructions are grouped into
+fixed-sized @defn{bundles}.  If the argument is zero, this mode is disabled
+(which is the default state).  If the argument it not zero, it gives the size
+of an instruction bundle as a power of two (as for the @code{.p2align}
+directive, @pxref{P2align}).
+
+A @defn{bundle} is simply a sequence of instructions that starts on an aligned
+boundary.  For example, if @var{abs-expr} is @code{5} then the bundle size is
+32, so each aligned chunk of 32 bytes is a bundle.  When aligned instruction
+bundle mode is in effect, no single instruction may span a boundary between
+bundles.  If an instruction would start too close to the end of a bundle for
+the length of that particular instruction to fit within the bundle, then the
+space at the end of that bundle is filled with no-op instructions so the
+instruction starts in the next bundle.  As a corollary, it's an error if any
+single instruction's encoding is longer than the bundle size.
+
+@section @code{.bundle_lock} and @code{.bundle_unlock}
+@cindex @code{bundle_lock} directive
+@cindex @code{bundle_unlock} directive
+The @code{.bundle_lock} and directive @code{.bundle_unlock} directives allow
+explicit control over instruction bundle padding.  These directives are only
+valid when @code{.bundle_align_mode} has been used to enable aligned
+instruction bundle mode.  It's an error if they appear when
+@code{.bundle_align_mode} has not been used at all, or when the last directive
+was @w{@code{.bundle_align_mode 0}}.
+
+@cindex bundle-locked
+A pair of @code{.bundle_lock} and @code{.bundle_unlock} directives define a
+@defn{bundle-locked} instruction sequence.  For purposes of aligned instruction
+bundle mode, a sequence starting with @code{.bundle_lock} and ending with
+@code{.bundle_unlock} is treated as a single instruction.  That is, the entire
+sequence must fit into a single bundle and may not span a bundle boundary.  If
+necessary, no-op instructions will be inserted before the first instruction of
+the sequence so that the whole sequence starts on an aligned bundle boundary.
+It's an error if the sequence is longer than the bundle size.
+
 @node Byte
 @section @code{.byte @var{expressions}}
 
diff --git a/gas/read.c b/gas/read.c
index 445caa1..df7702c 100644
--- a/gas/read.c
+++ b/gas/read.c
@@ -1,7 +1,5 @@
 /* read.c - read a source file -
-   Copyright 1986, 1987, 1990, 1991, 1992, 1993, 1994, 1995, 1996, 1997,
-   1998, 1999, 2000, 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009,
-   2010, 2011  Free Software Foundation, Inc.
+   Copyright 1986,1987,1990-2012 Free Software Foundation, Inc.
 
    This file is part of GAS, the GNU Assembler.
 
@@ -209,6 +207,19 @@ static int dwarf_file_string;
 #endif
 #endif
 
+#ifdef md_max_size
+# define HANDLE_BUNDLE
+#endif
+
+#ifdef HANDLE_BUNDLE
+/* .bundle_align_mode sets this.
+   XXX say more */
+static unsigned int bundle_align_p2;
+
+static frchainS *bundle_lock_frchain;
+static fragS *bundle_lock_frag;
+#endif
+
 static void do_s_func (int end_p, const char *default_prefix);
 static void do_align (int, char *, int, int);
 static void s_align (int, int);
@@ -277,6 +288,11 @@ static const pseudo_typeS potable[] = {
   {"balignw", s_align_bytes, -2},
   {"balignl", s_align_bytes, -4},
 /* block  */
+#ifdef HANDLE_BUNDLE
+  {"bundle_align_mode", s_bundle_align_mode, 0},
+  {"bundle_lock", s_bundle_lock, 0},
+  {"bundle_unlock", s_bundle_unlock, 0},
+#endif
   {"byte", cons, 1},
   {"comm", s_comm, 0},
   {"common", s_mri_common, 0},
@@ -583,6 +599,99 @@ try_macro (char term, const char *line)
   return 0;
 }
 
+#ifdef HANDLE_BUNDLE
+static unsigned int
+pending_bundle_size (fragS *frag)
+{
+  unsigned int size = 0;
+
+  while (frag != frag_now)
+    {
+      /* This should only happen in what will later become an error case.  */
+      if (frag == NULL)
+        return 0;
+
+      if (frag->fr_type == rs_machine_dependent)
+        size += md_max_size (frag);
+      else
+        size += frag->fr_fix;
+
+      frag = frag->fr_next;
+    }
+
+  size += frag_now_fix ();
+  return size;
+}
+
+static unsigned int
+close_bundle_frag (fragS *frag)
+{
+  unsigned int bundle_size = pending_bundle_size (frag);
+
+  if (bundle_size <= (1U << bundle_align_p2))
+    {
+      if (bundle_size > 1)
+        {
+          frag->fr_offset = bundle_align_p2;
+          frag->fr_subtype = bundle_size - 1;
+        }
+      return 0;
+    }
+
+  return bundle_size;
+}
+
+/* Assemble one instruction.  */
+static void
+assemble_one (char *line)
+{
+  fragS *insn_start_frag = frag_now;
+
+  if (bundle_lock_frchain != NULL && bundle_lock_frchain != frchain_now)
+    {
+      as_bad (_("cannot change section or subsection inside .bundle_lock"));
+      /* Clearing this serves as a marker that we have already complained.  */
+      bundle_lock_frchain = NULL;
+    }
+
+  if (bundle_lock_frag == NULL && bundle_align_p2 > 0)
+    frag_align_code (0, 0);
+
+  md_assemble (line);
+
+  if (bundle_lock_frchain != NULL)
+    {
+      /* Make sure this hasn't pushed the locked sequence
+         past the bundle size.  */
+      unsigned int bundle_size = pending_bundle_size (bundle_lock_frag);
+      if (bundle_size > (1U << bundle_align_p2))
+        as_bad (_("\
+.bundle_lock sequence at %u bytes but .bundle_align_mode limit is %u bytes"),
+                bundle_size, 1U << bundle_align_p2);
+    }
+  else if (bundle_align_p2 > 0)
+    {
+      addressT insn_size = pending_bundle_size (insn_start_frag);
+
+      if (insn_size > (1U << bundle_align_p2))
+        as_bad (_("\
+single instruction is %u bytes long but .bundle_align_mode limit is %u"),
+                (unsigned int) insn_size, 1U << bundle_align_p2);
+
+      if (insn_size > 1)
+        {
+          insn_start_frag->fr_offset = bundle_align_p2;
+          insn_start_frag->fr_subtype = insn_size - 1;
+        }
+    }
+}
+
+#else
+
+# define assemble_one(line) md_assemble(line)
+
+#endif
+
 /* We read the file, putting things into a web that represents what we
    have been reading.  */
 void
@@ -947,7 +1056,7 @@ read_a_source_file (char *name)
 			    }
 			}
 
-		      md_assemble (s);	/* Assemble 1 instruction.  */
+		      assemble_one (s); /* Assemble 1 instruction.  */
 
 		      *input_line_pointer++ = c;
 
@@ -1128,6 +1237,16 @@ read_a_source_file (char *name)
  quit:
   symbol_set_value_now (&dot_symbol);
 
+#ifdef HANDLE_BUNDLE
+  if (bundle_lock_frag != NULL)
+    {
+      as_bad_where (bundle_lock_frag->fr_file, bundle_lock_frag->fr_line,
+                    _(".bundle_lock with no matching .bundle_unlock"));
+      bundle_lock_frag = NULL;
+      bundle_lock_frchain = NULL;
+    }
+#endif
+
 #ifdef md_cleanup
   md_cleanup ();
 #endif
@@ -5867,6 +5986,78 @@ do_s_func (int end_p, const char *default_prefix)
   demand_empty_rest_of_line ();
 }
 
+#ifdef HANDLE_BUNDLE
+
+void
+s_bundle_align_mode (int arg ATTRIBUTE_UNUSED)
+{
+  unsigned int align = get_absolute_expression ();
+  SKIP_WHITESPACE ();
+  demand_empty_rest_of_line ();
+
+  if (align > (unsigned int) TC_ALIGN_LIMIT)
+    as_fatal (_("alignment too large"));
+
+  if (bundle_lock_frag != NULL)
+    {
+      as_bad (_("cannot change .bundle_align_mode inside .bundle_lock"));
+      return;
+    }
+
+  bundle_align_p2 = align;
+
+  if (align != 0)
+    record_alignment (now_seg, align - OCTETS_PER_BYTE_POWER);
+  /* XXX what about section changes? */
+}
+
+void
+s_bundle_lock (int arg ATTRIBUTE_UNUSED)
+{
+  demand_empty_rest_of_line ();
+
+  if (bundle_align_p2 == 0)
+    {
+      as_warn (_(".bundle_lock is meaningless without .bundle_align_mode"));
+      return;
+    }
+
+  if (bundle_lock_frag != NULL)
+    {
+      as_bad (_("second .bundle_lock without .bundle_unlock"));
+      return;
+    }
+
+  bundle_lock_frchain = frchain_now;
+  bundle_lock_frag = frag_now;
+  frag_align_code (0, 0);
+}
+
+void
+s_bundle_unlock (int arg ATTRIBUTE_UNUSED)
+{
+  unsigned int size;
+
+  demand_empty_rest_of_line ();
+
+  if (bundle_lock_frag == NULL)
+    {
+      as_bad (_(".bundle_unlock without preceding .bundle_lock"));
+      return;
+    }
+
+  gas_assert (bundle_align_p2 > 0);
+
+  size = close_bundle_frag (bundle_lock_frag);
+  bundle_lock_frag = NULL;
+  bundle_lock_frchain = NULL;
+  if (size > 0)
+    as_bad (_(".bundle_lock sequence is %u bytes, but bundle size only %u"),
+            size, 1 << bundle_align_p2);
+}
+
+#endif
+
 void
 s_ignore (int arg ATTRIBUTE_UNUSED)
 {
diff --git a/gas/read.h b/gas/read.h
index 82ccc75..ba43fa0 100644
--- a/gas/read.h
+++ b/gas/read.h
@@ -1,7 +1,5 @@
 /* read.h - of read.c
-   Copyright 1986, 1990, 1992, 1993, 1994, 1995, 1996, 1997, 1998, 1999,
-   2000, 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2009
-   Free Software Foundation, Inc.
+   Copyright 1986,1990,1992-2012 Free Software Foundation, Inc.
 
    This file is part of GAS, the GNU Assembler.
 
@@ -143,6 +141,9 @@ extern symbolS *s_lcomm_internal (int, symbolS *, addressT);
 extern void s_app_file_string (char *, int);
 extern void s_app_file (int);
 extern void s_app_line (int);
+extern void s_bundle_align_mode (int);
+extern void s_bundle_lock (int);
+extern void s_bundle_unlock (int);
 extern void s_comm (int);
 extern void s_data (int);
 extern void s_desc (int);