> On 2/7/24 1:45 PM, Yonghong Song wrote:
>>
>> On 2/7/24 2:12 AM, Jose E. Marchesi wrote:
>>> Some BPF tests use loop unrolling compiler pragmas that are clang
>>> specific and not supported by GCC. These pragmas, along with their
>>> GCC equivalences are:
>>>
>>> #pragma clang loop unroll_count(N)
>>> #pragma GCC unroll N
>>>
>>> #pragma clang loop unroll(full)
>>> #pragma GCC unroll 65534
>>>
>>> #pragma clang loop unroll(disable)
>>> #pragma GCC unroll 1
>>>
>>> #pragma unroll [aka #pragma clang loop unroll(enable)]
>>> There is no GCC equivalence, and it seems to me that this clang
>>> pragma may be only useful when building without -funroll-loops to
>>> enable the optimization in particular loops. In GCC -funroll-loops
>>> is enabled with -O2 and higher. If this is also true in clang,
>>> perhaps these pragmas in selftests are redundant?
>>
>> You are right, at -O2 level, loop unrolling is enabled by default.
>> So I think '#pragma unroll' can be removed since gcc also has
>> loop unrolling enabled by default at -O2.
>
> My comment in the above is not correct. In clang,
> at -O2 level, with and without "#pragma unroll", the generated
> code could be different. Basically "#pragma unroll" seems
> more aggressive in inlining compared to without it.
>
> So the current patch LGTM.
>
> Acked-by: Yonghong Song <yonghong.song@xxxxxxxxx>
I need to send a v2 with the conflict resolved.
>
>>
>> Your patch has a conflict with latest bpf-next. Please rebase it
>> on top of bpf-next, remove '#pragma unroll' support and resubmit.
>> Thanks!
>>
>>>
>>> This patch adds a new header progs/bpf_compiler.h that defines the
>>> following macros, which correspond to each pair of compiler-specific
>>> pragmas above:
>>>
>>> __pragma_loop_unroll_count(N)
>>> __pragma_loop_unroll_full
>>> __pragma_loop_no_unroll
>>> __pragma_loop_unroll
>>>
>>> The selftests using loop unrolling pragmas are then changed to include
>>> the header and use these macros in place of the explicit pragmas.
>>>
>>> Tested in bpf-next master.
>>> No regressions.
>>>
>>> Signed-off-by: Jose E. Marchesi <jose.marchesi@xxxxxxxxxx>
>>> Cc: Yonghong Song <yhs@xxxxxxxx>
>>> Cc: Eduard Zingerman <eddyz87@xxxxxxxxx>
>>> Cc: Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx>
>>> Cc: david.faust@xxxxxxxxxx
>>> Cc: cupertino.miranda@xxxxxxxxxx
>>> ---
>>> .../selftests/bpf/progs/bpf_compiler.h | 33 +++++++++++++++++++
>>> tools/testing/selftests/bpf/progs/iters.c | 5 +--
>>> tools/testing/selftests/bpf/progs/loop4.c | 4 ++-
>>> .../selftests/bpf/progs/profiler.inc.h | 17 +++++-----
>>> tools/testing/selftests/bpf/progs/pyperf.h | 7 ++--
>>> .../testing/selftests/bpf/progs/strobemeta.h | 18 +++++-----
>>> .../selftests/bpf/progs/test_cls_redirect.c | 5 +--
>>> .../selftests/bpf/progs/test_lwt_seg6local.c | 6 ++--
>>> .../selftests/bpf/progs/test_seg6_loop.c | 4 ++-
>>> .../selftests/bpf/progs/test_skb_ctx.c | 4 ++-
>>> .../selftests/bpf/progs/test_sysctl_loop1.c | 6 ++--
>>> .../selftests/bpf/progs/test_sysctl_loop2.c | 6 ++--
>>> .../selftests/bpf/progs/test_sysctl_prog.c | 6 ++--
>>> .../selftests/bpf/progs/test_tc_tunnel.c | 4 ++-
>>> tools/testing/selftests/bpf/progs/test_xdp.c | 3 +-
>>> .../selftests/bpf/progs/test_xdp_loop.c | 3 +-
>>> .../selftests/bpf/progs/test_xdp_noinline.c | 5 +--
>>> .../selftests/bpf/progs/xdp_synproxy_kern.c | 6 ++--
>>> .../testing/selftests/bpf/progs/xdping_kern.c | 3 +-
>>> 19 files changed, 103 insertions(+), 42 deletions(-)
>>> create mode 100644 tools/testing/selftests/bpf/progs/bpf_compiler.h
>>>
>>> diff --git a/tools/testing/selftests/bpf/progs/bpf_compiler.h
>>> b/tools/testing/selftests/bpf/progs/bpf_compiler.h
>>> new file mode 100644
>>> index 000000000000..a7c343dc82e6
>>> --- /dev/null
>>> +++ b/tools/testing/selftests/bpf/progs/bpf_compiler.h
>>> @@ -0,0 +1,33 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef __BPF_COMPILER_H__
>>> +#define __BPF_COMPILER_H__
>>> +
>>> +#define DO_PRAGMA_(X) _Pragma(#X)
>>> +
>>> +#if __clang__
>>> +#define __pragma_loop_unroll DO_PRAGMA_(clang loop unroll(enable))
>>> +#else
>>> +/* In GCC -funroll-loops, which is enabled with -O2, should have the
>>> + same impact than the loop-unroll-enable pragma above. */
>>> +#define __pragma_loop_unroll
>>> +#endif
>>> +
>>> +#if __clang__
>>> +#define __pragma_loop_unroll_count(N) DO_PRAGMA_(clang loop
>>> unroll_count(N))
>>> +#else
>>> +#define __pragma_loop_unroll_count(N) DO_PRAGMA_(GCC unroll N)
>>> +#endif
>>> +
>>> +#if __clang__
>>> +#define __pragma_loop_unroll_full DO_PRAGMA_(clang loop unroll(full))
>>> +#else
>>> +#define __pragma_loop_unroll_full DO_PRAGMA_(GCC unroll 65534)
>>> +#endif
>>> +
>>> +#if __clang__
>>> +#define __pragma_loop_no_unroll DO_PRAGMA_(clang loop unroll(disable))
>>> +#else
>>> +#define __pragma_loop_no_unroll DO_PRAGMA_(GCC unroll 1)
>>> +#endif
>>> +
>>> +#endif
>>> diff --git a/tools/testing/selftests/bpf/progs/iters.c
>>> b/tools/testing/selftests/bpf/progs/iters.c
>>> index 225f02dd66d0..3db416606f2f 100644
>>> --- a/tools/testing/selftests/bpf/progs/iters.c
>>> +++ b/tools/testing/selftests/bpf/progs/iters.c
>>> @@ -5,6 +5,7 @@
>>> #include <linux/bpf.h>
>>> #include <bpf/bpf_helpers.h>
>>> #include "bpf_misc.h"
>>> +#include "bpf_compiler.h"
>>> #define ARRAY_SIZE(x) (int)(sizeof(x) / sizeof((x)[0]))
>>> @@ -183,7 +184,7 @@ int iter_pragma_unroll_loop(const void *ctx)
>>> MY_PID_GUARD();
>>> bpf_iter_num_new(&it, 0, 2);
>>> -#pragma nounroll
>>> + __pragma_loop_no_unroll
>>> for (i = 0; i < 3; i++) {
>>> v = bpf_iter_num_next(&it);
>>> bpf_printk("ITER_BASIC: E3 VAL: i=%d v=%d", i, v ? *v : -1);
>>> @@ -238,7 +239,7 @@ int iter_multiple_sequential_loops(const void *ctx)
>>> bpf_iter_num_destroy(&it);
>>> bpf_iter_num_new(&it, 0, 2);
>>> -#pragma nounroll
>>> + __pragma_loop_no_unroll
>>> for (i = 0; i < 3; i++) {
>>> v = bpf_iter_num_next(&it);
>>> bpf_printk("ITER_BASIC: E3 VAL: i=%d v=%d", i, v ? *v : -1);
>>
>> [...]
>>
>>
