On Thu, Oct 4, 2012 at 8:38 PM, DJ Delorie <d...@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.
