This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: reverse bitfield patch


On Thu, Oct 4, 2012 at 8:38 PM, DJ Delorie <dj@redhat.com> wrote:
>
>> ChangeLog missing, new functions need a toplevel comment documenting
>> function, argument and return value as per coding conventions.
>
> Any review of the patch itself?  I know the overhead is not there...

Why do you need to change varasm.c at all?  The hunks seem to be
completely separate of the attribute.

I don't like that you post-process the layout.  How does reversal
inter-operate with all the other features we have, like strict volatile
bitfields?  I for example see

+         /* If the bitfield-order attribute has been used on this
+            structure, the fields might not be in bit-order.  In that
+            case, we need a separate representative for each
+            field.  */
+         else if (DECL_FIELD_OFFSET (field) < DECL_FIELD_OFFSET (repr)
+                  || (DECL_FIELD_OFFSET (field) == DECL_FIELD_OFFSET (repr)
+                      && DECL_FIELD_BIT_OFFSET (field) <
DECL_FIELD_BIT_OFFSET (repr)))
+           {
+             finish_bitfield_representative (repr, prev);
+             repr = start_bitfield_representative (field);
+           }

which will severely pessimize bitfield accesses to structs with the
bitfield-order attribute.


+
+  type_attr_list = tree_cons (get_identifier ("bit_order"),
+                             build_tree_list (NULL_TREE,
get_identifier (bit_order)),
+                             type_attr_list);
+
+  TYPE_ATTRIBUTES (node) = type_attr_list;
+}
+
+void
+rx_note_pragma_bitorder (char *mode)
+{
+  if (mode == NULL)

so you are supporting this as #pragma.  Which ends up tacking bit_order
to each type.  Rather than this, why not operate similar to the packed
pragma, thus, adjust a global variable in stor-layout.c.

+static tree
+handle_bitorder_attribute (tree *ARG_UNUSED (node), tree ARG_UNUSED (name),
+                          tree ARG_UNUSED (args),
+                          int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+  tree bmode;
+  const char *bname;
+
+  /* Allow no arguments to mean "native".  */
+  if (args == NULL_TREE)
+    return NULL_TREE;
+
+  bmode = TREE_VALUE (args);
+
+  bname = IDENTIFIER_POINTER (bmode);
+  if (strcmp (bname, "msb")
+      && strcmp (bname, "lsb")
+      && strcmp (bname, "swapped")
+      && strcmp (bname, "native"))
+    {
+      error ("%qE is not a valid bit_order - use lsb, msb, native, or
swapped", bmode);
+      *no_add_attrs = true;
+    }
+
+  return NULL_TREE;

I don't see a value in attaching 'native' or 'msb'/'lsb' if it is
equal to 'native'.  You
un-necessarily pessimize code generation (is different code even desired
for a "no-op" bit_order attribute?).

So no, I don't like this post-process layouting thing.  It's a layouting mode
so it should have effects at bitfield layout time.

Richard.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]