On Wed, Mar 6, 2024 at 5:26 AM Eduard Zingerman <eddyz87@xxxxxxxxx> wrote:
>
> On Tue, 2024-03-05 at 19:19 -0800, Alexei Starovoitov wrote:
> > From: Alexei Starovoitov <ast@xxxxxxxxxx>
> >
> > Use may_goto instruction to implement cond_break macro.
> > Ideally the macro should be written as:
> > asm volatile goto(".byte 0xe5;
> > .byte 0;
> > .short %l[l_break] ...
> > .long 0;
> > but LLVM doesn't recognize fixup of 2 byte PC relative yet.
> > Hence use
> > asm volatile goto(".byte 0xe5;
> > .byte 0;
> > .long %l[l_break] ...
> > .short 0;
> > that produces correct asm on little endian.
> >
> > Signed-off-by: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> Acked-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
>
> I tried rewriting with offset +1 and an additional goto:
>
> ({ __label__ l_break, l_continue; \
> asm volatile goto("%[jcond]; goto %l[l_break];"\
> :: __imm_insn(jcond, BPF_RAW_INSN(BPF_JMP | BPF_JCOND, 0, 0, 1, 0)) \
> :: l_break); \
> goto l_continue; \
> l_break: break; \
> l_continue:; \
> })
It probably works, but the generated code is not pretty.
That's the reason why this macro has two labels l_break and l_continue.
It could have been written with a single label, like:
/* bad asm */
#define cond_break \
({ __label__ l_continue; \
asm volatile goto("may_goto %l[l_continue]" :::: l_continue); \
break; \
l_continue: ; \
})
but generated code isn't great.
It's similar to bpf_cmp_likely vs bpf_cmp_unlikely.
The C statement that llvm sees right after 'asm volatile goto'
is important for codegen.
The macro in this patch is effectively this
/* good asm */
#define cond_break \
({ __label__ l_break, l_continue; \
asm volatile goto("may_goto %l[l_break]" :::: l_break); \
goto l_continue; \
l_break: break; \
l_continue: ; \
})
To visualize see:
https://godbolt.org/z/98bcbrxaE
The next step is to do several fixes in llvm:
- allow pcrel fixup of 2 bytes
- introduce builtin_may_goto()
- recognize may_goto in inline asm and llvm-objdump disasm
With that the macro will be cleaned up and the big-endian issue
will be addressed.
