On Fri, Dec 8, 2023 at 9:05 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
> On 12/8/23 8:25 PM, Alexei Starovoitov wrote:
> > On Fri, Dec 8, 2023 at 8:15 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
> >>
> >> On 12/8/23 8:05 PM, Alexei Starovoitov wrote:
> >>> On Fri, Dec 8, 2023 at 2:04 PM Andrii Nakryiko
> >>> <andrii.nakryiko@xxxxxxxxx> wrote:
> >>>> I feel like embedding some sort of ID inside the instruction is very..
> >>>> unusual, shall we say?
> >>> yeah. no magic numbers inside insns pls.
> >>>
> >>> I don't like JA_CFG name, since I read CFG as control flow graph,
> >>> while you probably meant CFG as configurable.
> >>> How about BPF_JA_OR_NOP ?
> >>> Then in combination with BPF_JMP or BPF_JMP32 modifier
> >>> the insn->off|imm will be used.
> >>> 1st bit in src_reg can indicate the default action: nop or jmp.
> >>> In asm it may look like asm("goto_or_nop +5")
> >> How does the C source code looks like in order to generate
> >> BPF_JA_OR_NOP insn? Any source examples?
> > It will be in inline asm only. The address of that insn will
> > be taken either via && or via asm (".long %l[label]").
> > From llvm pov both should go through the same relo creation logic. I hope :)
>
> A hack in llvm below with an example, could you check whether the C
> syntax and object dump result
> is what you want to see?
Thank you for the ultra quick llvm diff!
>
> diff --git a/llvm/lib/Target/BPF/AsmParser/BPFAsmParser.cpp
> b/llvm/lib/Target/BPF/AsmParser/BPFAsmParser.cpp
> index 90697c6645be..38b1cbc31f9a 100644
> --- a/llvm/lib/Target/BPF/AsmParser/BPFAsmParser.cpp
> +++ b/llvm/lib/Target/BPF/AsmParser/BPFAsmParser.cpp
> @@ -231,6 +231,7 @@ public:
> .Case("call", true)
> .Case("goto", true)
> .Case("gotol", true)
> + .Case("goto_or_nop", true)
> .Case("*", true)
> .Case("exit", true)
> .Case("lock", true)
> @@ -259,6 +260,7 @@ public:
> .Case("bswap64", true)
> .Case("goto", true)
> .Case("gotol", true)
> + .Case("goto_or_nop", true)
> .Case("ll", true)
> .Case("skb", true)
> .Case("s", true)
> diff --git a/llvm/lib/Target/BPF/BPFInstrInfo.td
> b/llvm/lib/Target/BPF/BPFInstrInfo.td
> index 5972c9d49c51..a953d10429bf 100644
> --- a/llvm/lib/Target/BPF/BPFInstrInfo.td
> +++ b/llvm/lib/Target/BPF/BPFInstrInfo.td
> @@ -592,6 +592,19 @@ class BRANCH<BPFJumpOp Opc, string OpcodeStr,
> list<dag> Pattern>
> let BPFClass = BPF_JMP;
> }
>
> +class BRANCH_OR_NOP<BPFJumpOp Opc, string OpcodeStr, list<dag> Pattern>
> + : TYPE_ALU_JMP<Opc.Value, BPF_K.Value,
> + (outs),
> + (ins brtarget:$BrDst),
> + !strconcat(OpcodeStr, " $BrDst"),
> + Pattern> {
> + bits<16> BrDst;
> +
> + let Inst{47-32} = BrDst;
> + let Inst{31-0} = 1;
> + let BPFClass = BPF_JMP;
> +}
> +
> class BRANCH_LONG<BPFJumpOp Opc, string OpcodeStr, list<dag> Pattern>
> : TYPE_ALU_JMP<Opc.Value, BPF_K.Value,
> (outs),
> @@ -632,6 +645,7 @@ class CALLX<string OpcodeStr>
> let isBranch = 1, isTerminator = 1, hasDelaySlot=0, isBarrier = 1 in {
> def JMP : BRANCH<BPF_JA, "goto", [(br bb:$BrDst)]>;
> def JMPL : BRANCH_LONG<BPF_JA, "gotol", []>;
> + def JMP_OR_NOP : BRANCH_OR_NOP<BPF_JA, "goto_or_nop", []>;
I was thinking of burning the new 0xE opcode for it,
but you're right. It's a flavor of existing JA insn and it's indeed
better to just use src_reg=1 bit to indicate so.
We probably need to use the 2nd bit of src_reg to indicate its default state
(jmp or fallthrough).
> asm volatile goto ("r0 = 0; \
> goto_or_nop %l[label]; \
> r2 = 2; \
> r3 = 3; \
Not sure how to represent the default state in assembly though.
"goto_or_nop" defaults to goto
"nop_or_goto" default to nop
?
Do we need "gotol" for imm32 or will it be automatic?
