On 18.12.20 08:54, Jan Beulich wrote:
On 18.12.2020 00:54, Stefano Stabellini wrote:On Tue, 15 Dec 2020, Jan Beulich wrote:On 15.12.2020 14:19, Julien Grall wrote:On 15/12/2020 11:46, Jan Beulich wrote:On 15.12.2020 12:26, Julien Grall wrote:--- a/xen/include/xen/lib.h +++ b/xen/include/xen/lib.h @@ -23,7 +23,13 @@ #include <asm/bug.h>#define BUG_ON(p) do { if (unlikely(p)) BUG(); } while (0)-#define WARN_ON(p) do { if (unlikely(p)) WARN(); } while (0) +#define WARN_ON(p) ({ \ + bool __ret_warn_on = (p); \Please can you avoid leading underscores here?I can.+ \ + if ( unlikely(__ret_warn_on) ) \ + WARN(); \ + unlikely(__ret_warn_on); \ +})Is this latter unlikely() having any effect? So far I thought it would need to be immediately inside a control construct or be an operand to && or ||.The unlikely() is directly taken from the Linux implementation. My guess is the compiler is still able to use the information for the branch prediction in the case of: if ( WARN_ON(...) )Maybe. Or maybe not. I don't suppose the Linux commit introducing it clarifies this?I did a bit of digging but it looks like the unlikely has been there forever. I'd just keep it as is.I'm afraid I don't view this as a reason to inherit code unchanged. If it was introduced with a clear indication that compilers can recognize it despite the somewhat unusual placement, then fine. But likely() / unlikely() quite often get put in more or less blindly - see the not uncommon unlikely(a && b) style of uses, which don't typically have the intended effect and would instead need to be unlikely(a) && unlikely(b) [assuming each condition alone is indeed deemed unlikely], unless compilers have learned to guess/infer what's meant between when I last looked at this and now.
I have made a little experiment and found that the unlikely() at the end of a macro is having effect. The disassembly of #define unlikely(x) __builtin_expect(!!(x), 0) #define foo() ({ \ int i = !time(NULL); \ unlikely(i); }) #include "stdio.h" #include "time.h" int main() { if (foo()) puts("a"); return 0; } is the same as that of: #define unlikely(x) __builtin_expect(!!(x), 0) #include "stdio.h" #include "time.h" int main() { int i = !time(NULL); if (unlikely(i)) puts("a"); return 0; } while that of: #include "stdio.h" #include "time.h" int main() { int i = !time(NULL); if (i) puts("a"); return 0; } is different. Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys
OpenPGP_signature
Description: OpenPGP digital signature