| Message ID | 502AC3DC.9090102@mellanox.co.il (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Alex Netes |
| Headers | show |
> +#define if_PF(cond) if(CL_PREDICT_FALSE(cond)) > +#define if_PT(cond) if(CL_PREDICT_TRUE(cond)) If CL_PREDICT_TRUE/FALSE are too long, why not just shorten those, rather than abstract if statements behind a macro? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Aug 14, 2012 at 09:39:23PM +0000, Hefty, Sean wrote: > > +#define if_PF(cond) if(CL_PREDICT_FALSE(cond)) > > +#define if_PT(cond) if(CL_PREDICT_TRUE(cond)) > > If CL_PREDICT_TRUE/FALSE are too long, why not just shorten those, > rather than abstract if statements behind a macro? If you are typing these so often (ie more than in some datastructure kernels) that the length is a problem then you really should be using profile guided optimization instead... Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Sorry for the delay with a response - just got back from a long vacation. On 8/15/2012 12:54 AM, Jason Gunthorpe wrote: > On Tue, Aug 14, 2012 at 09:39:23PM +0000, Hefty, Sean wrote: >>> +#define if_PF(cond) if(CL_PREDICT_FALSE(cond)) >>> +#define if_PT(cond) if(CL_PREDICT_TRUE(cond)) >> >> If CL_PREDICT_TRUE/FALSE are too long, why not just shorten those, >> rather than abstract if statements behind a macro? It's not the size - it's just the readability of the macro. First macro (CL_PREDICT_*) is close to the GCC's way to define it. Second macro (if_PF/T) is close to the usual 'if' statement syntax. If you think it's a deal breaker, I can lose the first macro and have everything in one macro. > If you are typing these so often (ie more than in some datastructure > kernels) that the length is a problem then you really should be using > profile guided optimization instead... I'm trying to optimize all the 'if' conditions in the SM. As I described in patch 3/8 (next patch in these series), I don't touch all the non-fatal cases. The goal is not to help SM to perform better in general (in which case I agree than PGO would be the right way to go), but to make sure that the fatal cases will never be predicted wrong, so any fatal check (existing or a newly added one) won't affect the SM performance. Also, the plan is to define a new ASSERT macro that will use this if_PF thing to make sure that ASSERT statement doesn't cost much (if any). -- YK > Jason > -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 8/26/2012 4:43 PM, Yevgeny Kliteynik wrote: > >> If you are typing these so often (ie more than in some datastructure >> kernels) that the length is a problem then you really should be using >> profile guided optimization instead... > > I'm trying to optimize all the 'if' conditions in the SM. Obviously, what I meant is that I'm *not* trying to optimize all the 'if' conditions in the SM... :) -- YK -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On 8/15/2012 12:54 AM, Jason Gunthorpe wrote: > > On Tue, Aug 14, 2012 at 09:39:23PM +0000, Hefty, Sean wrote: > >>> +#define if_PF(cond) if(CL_PREDICT_FALSE(cond)) > >>> +#define if_PT(cond) if(CL_PREDICT_TRUE(cond)) > >> > >> If CL_PREDICT_TRUE/FALSE are too long, why not just shorten those, > >> rather than abstract if statements behind a macro? > > It's not the size - it's just the readability of the macro. > First macro (CL_PREDICT_*) is close to the GCC's way to define > it. Second macro (if_PF/T) is close to the usual 'if' statement > syntax. I would not abstract the 'if' statement. If CL_PREDICT_FALSE/TRUE are not readable, then shorten those. if (PF(...)) is just as readable as if_PF(...) -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > I would not abstract the 'if' statement. If CL_PREDICT_FALSE/TRUE are not readable, then shorten those. > > if (PF(...)) > > is just as readable as > > if_PF(...) OK, agree. I'll issue a v2 shortly - the only difference would be change in this macro and rebase to the updated trunk. -- YK -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/complib/cl_types_osd.h b/include/complib/cl_types_osd.h index ce1a452..f9abb28 100644 --- a/include/complib/cl_types_osd.h +++ b/include/complib/cl_types_osd.h @@ -1,6 +1,6 @@ /* * Copyright (c) 2004-2009 Voltaire, Inc. All rights reserved. - * Copyright (c) 2002-2005 Mellanox Technologies LTD. All rights reserved. + * Copyright (c) 2002-2012 Mellanox Technologies LTD. All rights reserved. * Copyright (c) 1996-2003 Intel Corporation. All rights reserved. * * This software is available to you under a choice of one of two @@ -64,6 +64,21 @@ BEGIN_C_DECLS #include <inttypes.h> #include <assert.h> #include <string.h> + +/* + * Branch prediction hints + */ +#if defined(HAVE_BUILTIN_EXPECT) +#define CL_PREDICT_TRUE(exp) __builtin_expect( ((uintptr_t)(exp)), 1 ) +#define CL_PREDICT_FALSE(exp) __builtin_expect( ((uintptr_t)(exp)), 0 ) +#else +#define CL_PREDICT_TRUE(exp) (exp) +#define CL_PREDICT_FALSE(exp) (exp) +#endif + +#define if_PF(cond) if(CL_PREDICT_FALSE(cond)) +#define if_PT(cond) if(CL_PREDICT_TRUE(cond)) + #if defined (_DEBUG_) #define CL_ASSERT assert #else /* _DEBUG_ */
Defined "if_PT" and "if_PF" for "predict true" and "predict false" respectively. Signed-off-by: Yevgeny Kliteynik <kliteyn@dev.mellanox.co.il> --- include/complib/cl_types_osd.h | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)