x86: bpf_jit_comp: simplify trivial boolean return
Subsystems (5)
Quoted 26 lines · Joe Perches
Quoted 4 lines · Alexei Starovoitov
Quoted 2 lines · Quentin Lambert
Remove if then else statements preceding
boolean return.[]
Quoted 1 line
arch/x86/net/bpf_jit_comp.c
Quoted 1 line
arch/x86/net/bpf_jit_comp.c +0
-0[]
Quoted 9 lines
Quoted 9 lines
arch/x86/net/bpf_jit_comp.c +0
-5@@ -135,11 +135,9 @@ static const int reg2hex[] = {*/ static inline bool is_ereg (u32 reg) { -if (reg ==BPF_REG_5 || reg ==AUX_REG || - (reg >=BPF_REG_7 && reg <=BPF_REG_9 )) -return true; -else -return false;Quoted 8 lines
arch/x86/net/bpf_jit_comp.c static const int reg2hex[] = { · line 140
Quoted 3 lines
arch/x86/net/bpf_jit_comp.c +3
-0+return (reg ==BPF_REG_5 || + reg ==AUX_REG || + (reg >=BPF_REG_7 && reg <=BPF_REG_9 ));please remove extra () around the whole expression, and align in properly, and
don't move reg==AUX_REG check to a different line. Subject is not warranted. I don't think it's a simplification.
Quoted 2 lines
It's not really a simplification,
gcc should emit the same object code.
exactly.
Quoted 6 lines
Quoted 3 lines
imo existing code is fine and I don't think the time spent reviewing such changes is worth it when there is no improvement in readability.
Is there any value in reordering these tests for frequency or maybe using | instead of || to avoid multiple jumps?
probably not. It's not a critical path.
compiler may fuse conditions depending on values anyway. If it was a critical path, we could have used
(1 << reg) & mask trick.
I picked explicit 'return true' else 'return false' here, because it felt easier to read. Just a matter of taste.
Quoted 11 lines · Alexei Starovoitov
Quoted 2 lines · Joe Perches
Is there any value in reordering these tests for frequency or maybe using | instead of || to avoid multiple jumps?
probably not. It's not a critical path.
compiler may fuse conditions depending on values anyway. If it was a critical path, we could have used
(1 << reg) & mask trick.
I picked explicit 'return true' else 'return false' here, because it felt easier to read. Just a matter of taste.
There is a size difference though: (allyesconfig)
$ size arch/x86/net/built-in.o* text data bss dec hex filename 12999 1012 4336 18347 47ab arch/x86/net/built-in.o.new 13177 1076 4592 18845 499d arch/x86/net/built-in.o.old ---
arch/x86/net/bpf_jit_comp.c +5 -5
@@ -135,11 +135,11 @@ static const int reg2hex[] = {*/ static inline bool is_ereg (u32 reg) { -if (reg ==BPF_REG_5 || reg ==AUX_REG || - (reg >=BPF_REG_7 && reg <=BPF_REG_9 )) -return true; -else -return false; +return (1 << reg) & (BIT (BPF_REG_5 ) | +BIT (AUX_REG ) | +BIT (BPF_REG_7 ) | +BIT (BPF_REG_8 ) | +BIT (BPF_REG_9 )); }/* add modifiers if 'reg' maps to x64 registers r8..r15 */
PATCH 1/1 x86: bpf_jit_comp: Reduce is_ereg() code size
In reply to Alexei StarovoitovUse the (1 << reg) & mask trick to reduce code size.
x86-64 size difference -O2 without profiling for various gcc versions:
$ size arch/x86/net/bpf_jit_comp.o* text data bss dec hex filename 9266 4 0 9270 2436 arch/x86/net/bpf_jit_comp.o.4.4.new 10042 4 0 10046 273e arch/x86/net/bpf_jit_comp.o.4.4.old 9109 4 0 9113 2399 arch/x86/net/bpf_jit_comp.o.4.6.new 9717 4 0 9721 25f9 arch/x86/net/bpf_jit_comp.o.4.6.old 8789 4 0 8793 2259 arch/x86/net/bpf_jit_comp.o.4.7.new 10245 4 0 10249 2809 arch/x86/net/bpf_jit_comp.o.4.7.old 9671 4 0 9675 25cb arch/x86/net/bpf_jit_comp.o.4.9.new 10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.4.9.old
---
compiled, untested by me, but per Alexei Starovoitov this passes the test_bpf suite
arch/x86/net/bpf_jit_comp.c +5 -5
@@ -135,11 +135,11 @@ static const int reg2hex[] = {*/ static inline bool is_ereg (u32 reg) { -if (reg ==BPF_REG_5 || reg ==AUX_REG || - (reg >=BPF_REG_7 && reg <=BPF_REG_9 )) -return true; -else -return false; +return (1 << reg) & (BIT (BPF_REG_5 ) | +BIT (AUX_REG ) | +BIT (BPF_REG_7 ) | +BIT (BPF_REG_8 ) | +BIT (BPF_REG_9 )); }/* add modifiers if 'reg' maps to x64 registers r8..r15 */
Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
Quoted 16 lines · Joe Perches
Use the (1 << reg) & mask trick to reduce code size.
x86-64 size difference -O2 without profiling for various gcc versions:
$ size arch/x86/net/bpf_jit_comp.o* text data bss dec hex filename 9266 4 0 9270 2436 arch/x86/net/bpf_jit_comp.o.4.4.new 10042 4 0 10046 273e arch/x86/net/bpf_jit_comp.o.4.4.old 9109 4 0 9113 2399 arch/x86/net/bpf_jit_comp.o.4.6.new 9717 4 0 9721 25f9 arch/x86/net/bpf_jit_comp.o.4.6.old 8789 4 0 8793 2259 arch/x86/net/bpf_jit_comp.o.4.7.new 10245 4 0 10249 2809 arch/x86/net/bpf_jit_comp.o.4.7.old 9671 4 0 9675 25cb arch/x86/net/bpf_jit_comp.o.4.9.new 10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.4.9.old
Quoted 1 line
Signed-off-by Joe Perches <[email protected]>
probably it was worth noting in comment that
reg is 4-bit value and AUX_REG==12, so it won't overflow.
Dave, it's for net-next of course.
Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
In reply to Joe PerchesQuoted 11 lines · Joe Perches
Use the (1 << reg) & mask trick to reduce code size.
x86-64 size difference -O2 without profiling for various gcc versions:
Signed-off-by Joe Perches <[email protected]>---
compiled, untested by me, but per Alexei Starovoitov this passes the
test_bpfsuite
Really, the root cause of this is the 'inline' abuse in non fast paths for non trivial functions.
Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
Quoted 7 lines · Eric Dumazet
Quoted 6 lines · Joe Perches
Use the (1 << reg) & mask trick to reduce code size.
x86-64 size difference -O2 without profiling for various gcc versions:
Quoted 8 lines
Quoted 5 lines
Signed-off-by Joe Perches <[email protected]>---
compiled, untested by me, but per Alexei Starovoitov this passes the
test_bpfsuiteReally, the root cause of this is the 'inline' abuse in non fast paths for non trivial functions.
well, it is a trivial function even from compiler point of view. Dropping inline keyword doesn't help. gcc still inlines them. Changing all 3 functions to _noinline_ doesn't help either. So I think this patch is actually quite helpful to reduce code size.
Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
Quoted 4 lines · Alexei Starovoitov
well, it is a trivial function even from compiler point of view. Dropping inline keyword doesn't help. gcc still inlines them. Changing all 3 functions to _noinline_ doesn't help either. So I think this patch is actually quite helpful to reduce code size.
Well, again this might depend on CONFIG_CC_OPTIMIZE_FOR_SIZE
I guess people trying to get very small kernels are using this option.
My point was : If we care about code size, we should also remove these inline keywords at the same time, to increase SNR of netdev/lkml lists.
PATCH 1/1 x86: bpf_jit_comp: Remove inline from static function definitions
Let the compiler decide instead.
No change in object size x86-64 -O2 no profiling
---
Quoted 8 lines · Eric Dumazet
Quoted 4 lines · Alexei Starovoitov
well, it is a trivial function even from compiler point of view. Dropping inline keyword doesn't help. gcc still inlines them. Changing all 3 functions to _noinline_ doesn't help either. So I think this patch is actually quite helpful to reduce code size.
Well, again this might depend on
CONFIG_CC_OPTIMIZE_FOR_SIZE
-Os has a different size delta, but it's still
smaller using this new function.
Quoted 4 lines
I guess people trying to get very small kernels are using this option.
My point was : If we care about code size, we should also remove these inline keywords at the same time, to increase SNR of netdev/lkml lists.
Because there's no object change here, inline removals would probably be a good thing for this file.
arch/x86/net/bpf_jit_comp.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
arch/x86/net/bpf_jit_comp.c +9 -9
@@ -24,7 +24,7 @@ extern u8 sk_load_byte_positive_offset[];extern u8 sk_load_word_negative_offset[], sk_load_half_negative_offset[];extern u8 sk_load_byte_negative_offset[]; -static inline u8 *emit_code (u8 *ptr,u32 bytes,unsigned int len) +static u8 *emit_code (u8 *ptr,u32 bytes,unsigned int len) {if (len ==1 ) *ptr = bytes;
@@ -52,12 +52,12 @@ static inline u8 *emit_code(u8 *ptr, u32 bytes, unsigned int len)#define EMIT4_off32 (b1, b2, b3, b4, off) \ do {EMIT4(b1, b2, b3, b4); EMIT(off, 4); } while (0) -static inline bool is_imm8 (int value) +static bool is_imm8 (int value) {return value <=127 && value >=-128 ; } -static inline bool is_simm32 (s64 value) +static bool is_simm32 (s64 value) {return value == (s64 ) (s32 ) value; }
@@ -94,7 +94,7 @@ static int bpf_size_to_x86_bytes(int bpf_size)#define X86_JGE 0x7D#define X86_JG 0x7F -static inline void bpf_flush_icache (void *start,void *end) +static void bpf_flush_icache (void *start,void *end) {mm_segment_t old_fs =get_fs ();
@@ -133,7 +133,7 @@ static const int reg2hex[] = {* which need extra byte of encoding. * rax,rcx,...,rbp have simpler encoding */ -static inline bool is_ereg (u32 reg) +static bool is_ereg (u32 reg) { return (1 << reg) & (BIT(BPF_REG_5) | BIT(AUX_REG) |
@@ -143,14 +143,14 @@ static inline bool is_ereg(u32 reg) }/* add modifiers if 'reg' maps to x64 registers r8..r15 */ -static inline u8 add_1mod (u8 byte,u32 reg) +static u8 add_1mod (u8 byte,u32 reg) {if (is_ereg (reg)) byte |=1 ;return byte; } -static inline u8 add_2mod (u8 byte,u32 r1,u32 r2) +static u8 add_2mod (u8 byte,u32 r1,u32 r2) {if (is_ereg (r1)) byte |=1 ;
@@ -160,13 +160,13 @@ static inline u8 add_2mod(u8 byte, u32 r1, u32 r2) }/* encode 'dst_reg' register into x64 opcode 'byte' */ -static inline u8 add_1reg (u8 byte,u32 dst_reg) +static u8 add_1reg (u8 byte,u32 dst_reg) {return byte + reg2hex[dst_reg]; }/* encode 'dst_reg' and 'src_reg' registers into x64 opcode 'byte' */ -static inline u8 add_2reg (u8 byte,u32 dst_reg,u32 src_reg) +static u8 add_2reg (u8 byte,u32 dst_reg,u32 src_reg) {return byte + reg2hex[dst_reg] + (reg2hex[src_reg] <<3 ); }
Re: [PATCH] x86: bpf_jit_comp: Remove inline from static function definitions
Quoted 6 lines · Joe Perches
Let the compiler decide instead.
No change in object size x86-64 -O2 no profiling
Signed-off-by Joe Perches <[email protected]>Suggested-by Eric Dumazet <[email protected]>
Dave, this is on top of previous patch:
http://patchwork.ozlabs.org/patch/417960/
Joe, please mention dependencies next time.
Re: [PATCH] x86: bpf_jit_comp: Remove inline from static function definitions
Quoted 1 line · Alexei Starovoitov
Joe, please mention dependencies next time.
No real need here.
It's in the same thread.
Re: [PATCH] x86: bpf_jit_comp: Remove inline from static function definitions
In reply to Joe PerchesFrom: Joe Perches <[email protected]>
Date: Thu, 04 Dec 2014 17:01:24 -0800
Quoted 6 lines
Let the compiler decide instead.
No change in object size x86-64 -O2 no profiling
Signed-off-by Joe Perches <[email protected]>Suggested-by Eric Dumazet <[email protected]>
Applied.
Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
In reply to Eric DumazetQuoted 2 lines · Eric Dumazet
Quoted 1 line · Joe Perches
Use the (1 << reg) & mask trick to reduce code size.
[]
Quoted 2 lines
Really, the root cause of this is the 'inline' abuse in non fast paths for non trivial functions.
There is no object size change with is_ereg()
defined "static inline" or "static"
Curiously, if you mark it noinline, the size increases.
gcc 4.9.1, x86-64, -O2 no profiling support
$ size arch/x86/net/bpf_jit_comp.o.st* text data bss dec hex filename 10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.static_inline 11535 4 0 11539 2d13 arch/x86/net/bpf_jit_comp.o.static_noinline 10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.static_without_inline
Re: [PATCH] x86: bpf_jit_comp: Reduce is_ereg() code size
In reply to Joe PerchesFrom: Joe Perches <[email protected]>
Date: Thu, 04 Dec 2014 15:00:48 -0800
Quoted 16 lines
Use the (1 << reg) & mask trick to reduce code size.
x86-64 size difference -O2 without profiling for various gcc versions:
$ size arch/x86/net/bpf_jit_comp.o* text data bss dec hex filename 9266 4 0 9270 2436 arch/x86/net/bpf_jit_comp.o.4.4.new 10042 4 0 10046 273e arch/x86/net/bpf_jit_comp.o.4.4.old 9109 4 0 9113 2399 arch/x86/net/bpf_jit_comp.o.4.6.new 9717 4 0 9721 25f9 arch/x86/net/bpf_jit_comp.o.4.6.old 8789 4 0 8793 2259 arch/x86/net/bpf_jit_comp.o.4.7.new 10245 4 0 10249 2809 arch/x86/net/bpf_jit_comp.o.4.7.old 9671 4 0 9675 25cb arch/x86/net/bpf_jit_comp.o.4.9.new 10679 4 0 10683 29bb arch/x86/net/bpf_jit_comp.o.4.9.old
Quoted 1 line
Signed-off-by Joe Perches <[email protected]>
Applied.