Re: [RFC PATCH] tg3: Convert chip type macros to inline functions

Joe Perches
2013-02-12T01:43:14Z

On Sun, 2013-02-10 at 17:34 -0800, Joe Perches wrote:
> On Sun, 2013-02-10 at 19:39 -0500, David Miller wrote:
> > From: Joe Perches <joe@perches.com>
> > Date: Wed, 06 Feb 2013 15:44:58 -0800
> > > To me the negative to these conversions is at
> > > least for gcc 4.7.2, the overall code size
> > > increases
> > > $ size drivers/net/ethernet/broadcom/tg3.o*
> > > text data bss dec hex filename
> > > 203426 13446 55744 272616 428e8 drivers/net/ethernet/broadcom/tg3.o.new
> > > 202135 13446 55144 270725 42185 drivers/net/ethernet/broadcom/tg3.o.old
> > > I'm not sure why gcc doesn't do the optimization
> > > and code generation the same way.
> > Out of curiosity I looked at the assembler difference on sparc
> > and one thing stood out.
[]
> > The assembler is hard to compare manually, because the inlining
> > changes how hard registers are allocated, and issues like the above
> > will change all of the branch and file offsets as well. :-/
[]
> If the inlines return int instead of u32, x86 objects
> do change trivially (using 2 jg vs ja) , but the size
> is unchanged from u32 to int (still bigger).

Another option would be to change the macro names
and add comments for why these aren't static inlines.

btw: I tried statement expressions too.

Simple statement expressions are the same size.
Oddly, SE macros like:

#define tg3_asic_rev(tp) \
({ \
typecheck(struct tg3 *, tp); \
((tp)->pci_chip_rev_id >> 12); \
})

produce larger code even though I expected the
optimizer to effectively remove the typecheck.

Maybe something like:

drivers/net/ethernet/broadcom/tg3.h | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index ef6ced2..2613691 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -120,9 +120,6 @@
#define MISC_HOST_CTRL_TAGGED_STATUS 0x00000200
#define MISC_HOST_CTRL_CHIPREV 0xffff0000
#define MISC_HOST_CTRL_CHIPREV_SHIFT 16
-#define GET_CHIP_REV_ID(MISC_HOST_CTRL) \
- (((MISC_HOST_CTRL) & MISC_HOST_CTRL_CHIPREV) >> \
- MISC_HOST_CTRL_CHIPREV_SHIFT)
#define CHIPREV_ID_5700_A0 0x7000
#define CHIPREV_ID_5700_A1 0x7001
#define CHIPREV_ID_5700_B0 0x7100
@@ -163,7 +160,7 @@
#define CHIPREV_ID_5719_A0 0x05719000
#define CHIPREV_ID_5720_A0 0x05720000
#define CHIPREV_ID_5762_A0 0x05762000
-#define GET_ASIC_REV(CHIP_REV_ID) ((CHIP_REV_ID) >> 12)
+
#define ASIC_REV_5700 0x07
#define ASIC_REV_5701 0x00
#define ASIC_REV_5703 0x01
@@ -187,7 +184,6 @@
#define ASIC_REV_5720 0x5720
#define ASIC_REV_57766 0x57766
#define ASIC_REV_5762 0x5762
-#define GET_CHIP_REV(CHIP_REV_ID) ((CHIP_REV_ID) >> 8)
#define CHIPREV_5700_AX 0x70
#define CHIPREV_5700_BX 0x71
#define CHIPREV_5700_CX 0x72
@@ -200,7 +196,6 @@
#define CHIPREV_5784_AX 0x57840
#define CHIPREV_5761_AX 0x57610
#define CHIPREV_57765_AX 0x577650
-#define GET_METAL_REV(CHIP_REV_ID) ((CHIP_REV_ID) & 0xff)
#define METAL_REV_A0 0x00
#define METAL_REV_A1 0x01
#define METAL_REV_B0 0x00
@@ -3357,4 +3352,23 @@ struct tg3 {
bool link_up;
};

+/* Accessor macros for chip and asic attributes
+ *
+ * nb: Using static inlines equivalent to the accessor macros generates
+ * larger object code with gcc 4.7.
+ * Using statement expression macros to check tp with
+ * typecheck(struct tg3 *, tp) also creates larger objects.
+ */
+#define tg3_chip_rev_id(tp) \
+ ((tp)->pci_chip_rev_id)
+#define tg3_asic_rev(tp) \
+ ((tp)->pci_chip_rev_id >> 12)
+#define tg3_chip_rev(tp) \
+ ((tp)->pci_chip_rev_id >> 8)
+#define tg3_metal_rev(tp) \
+ ((tp)->pci_chip_rev_id & 0xff)
+#define tg3_misc_host_ctrl(tp) \
+ (((tp)->misc_host_ctrl & MISC_HOST_CTRL_CHIPREV) >> \
+ MISC_HOST_CTRL_CHIPREV_SHIFT)
+
#endif /* !(_T3_H) */