[PATCH] [RFC PATCH] BPF backend

[Alexei Starovoitov]

On Wed, Dec 3, 2014 at 1:53 PM, Tom Stellard <thomas.stellard at amd.com> wrote:
> ================
> Comment at: configure:5309-5315
> @@ -5309,7 +5309,7 @@
>
>  fi
>
> -ALL_TARGETS="X86 Sparc PowerPC ARM AArch64 Mips XCore MSP430 CppBackend NVPTX Hexagon SystemZ R600"
> +ALL_TARGETS="X86 Sparc PowerPC ARM AArch64 Mips XCore MSP430 CppBackend NVPTX Hexagon SystemZ R600 BPF"
>  ALL_TARGETS=$ALL_TARGETS
>
>
> ----------------
> You need to add this target to the CMake build system too.

ok. will add cmake bits as well.

> ================
> Comment at: lib/Target/BPF/BPFISelLowering.cpp:434-435
> @@ +433,4 @@
> +
> +static bool NegateCC(SDValue &LHS, SDValue &RHS, ISD::CondCode &CC)
> +{
> +  switch (CC) {
> ----------------
> Coding style: Bracket should be on same line as function.

ohh yes. will do.

> ================
> Comment at: lib/Target/BPF/BPFISelLowering.cpp:436-456
> @@ +435,23 @@
> +{
> +  switch (CC) {
> +  default:
> +    return false;
> +  case ISD::SETULT:
> +    CC = ISD::SETUGT;
> +    std::swap(LHS, RHS);
> +    return true;
> +  case ISD::SETULE:
> +    CC = ISD::SETUGE;
> +    std::swap(LHS, RHS);
> +    return true;
> +  case ISD::SETLT:
> +    CC = ISD::SETGT;
> +    std::swap(LHS, RHS);
> +    return true;
> +  case ISD::SETLE:
> +    CC = ISD::SETGE;
> +    std::swap(LHS, RHS);
> +    return true;
> +  }
> +}
> +
> ----------------
> You may be able to simplify this using ISD::getSetCCSwappedOperands().

yes. good point. will do

> ================
> Comment at: lib/Target/BPF/BPFInstrInfo.td:96-99
> @@ +95,6 @@
> +// jump instructions
> +class JMP_RR<bits<4> br_op, string asmstr, PatLeaf Cond>
> +  : InstBPF<(outs), (ins GPR:$rA, GPR:$rX, brtarget:$dst),
> +           !strconcat(asmstr, "\t$rA, $rX goto $dst"),
> +           [(BPFbrcc (i64 GPR:$rA), (i64 GPR:$rX), Cond, bb:$dst)]> {
> +  bits<4> op;
> ----------------
> You don't need to specify register classes any more, so you can rewrite this pattern as:
>
> (BPFbrcc i64:$rA, i64:$rX, Cond, bb:$dst)]
>
> Some comment for most other patterns in this file.

ok. makes sense. that will definitely make it less verbose.

> ================
> Comment at: lib/Target/BPF/BPFInstrInfo.td:501-504
> @@ +500,6 @@
> +
> +class LOAD_IND<bits<2> sizeOp, string asmstr, Intrinsic opNode>
> +  : InstBPF<(outs), (ins GPR:$skb, GPR:$val),
> +            !strconcat(asmstr, "\tr0, $skb.data + $val"),
> +            [(set R0, (opNode GPR:$skb, GPR:$val))]> {
> +  bits<3> mode;
> ----------------
> I did not realize this was possible.  Does the instruction selector add R0 as an implicit def?

yep. it looks like this:
 LD_ABS_H %R6, 12, %R0<imp-def>, %R1<imp-def,dead>, %R2<imp-def,dead>,
%R3<imp-def,dead>, %R4<imp-def,dead>, %R5<imp-def,dead>, %R6<imp-use>
 %vreg57<def> = COPY %R0; GPR:%vreg57

I'm not sure it's documented, since I don't see other backends use it.
Should I change to something else?
What's a recommended approach?

> ================
> Comment at: lib/Target/BPF/BPFRegisterInfo.td:7-8
> @@ +6,4 @@
> +
> +class BPFReg<string n> : Register<n> {
> +  field bits<4> Num;
> +  let Namespace = "BPF";
> ----------------
> I'm not sure what you are using Num here for, but Register  has a HWEncoding field which is accessible from the CodeEmitter if you need it.

left over of copy-paste. will remove.

> ================
> Comment at: lib/Target/BPF/MCTargetDesc/BPFBaseInfo.h:14-30
> @@ +13,19 @@
> +
> +static inline unsigned getBPFRegisterNumbering(unsigned Reg) {
> +  switch(Reg) {
> +    case BPF::R0  : return 0;
> +    case BPF::R1  : return 1;
> +    case BPF::R2  : return 2;
> +    case BPF::R3  : return 3;
> +    case BPF::R4  : return 4;
> +    case BPF::R5  : return 5;
> +    case BPF::R6  : return 6;
> +    case BPF::R7  : return 7;
> +    case BPF::R8  : return 8;
> +    case BPF::R9  : return 9;
> +    case BPF::R10 : return 10;
> +    case BPF::R11 : return 11;
> +    default: llvm_unreachable("Unknown register number!");
> +  }
> +}
> +
> ----------------
> See my previous comment about the HWEncoding field for registers, you can have TableGen generate this for you.

ok. will do. When I started that's how other backends were doing it.

> ================
> Comment at: lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp:127-147
> @@ +126,23 @@
> +
> +  if (Opcode == BPF::LD_imm64) {
> +    uint64_t Value = getBinaryCodeForInstr(MI, Fixups, STI);
> +    EmitByte(Value >> 56, CurByte, OS);
> +    EmitByte(((Value >> 48) & 0xff), CurByte, OS);
> +    EmitLEConstant(0, 2, CurByte, OS);
> +    EmitLEConstant(Value & 0xffffFFFF, 4, CurByte, OS);
> +
> +    const MCOperand &MO = MI.getOperand(1);
> +    uint64_t Imm = MO.isImm() ? MO.getImm() : 0;
> +    EmitByte(0, CurByte, OS);
> +    EmitByte(0, CurByte, OS);
> +    EmitLEConstant(0, 2, CurByte, OS);
> +    EmitLEConstant(Imm >> 32, 4, CurByte, OS);
> +  } else {
> +    // Get instruction encoding and emit it
> +    uint64_t Value = getBinaryCodeForInstr(MI, Fixups, STI);
> +    EmitByte(Value >> 56, CurByte, OS);
> +    EmitByte((Value >> 48) & 0xff, CurByte, OS);
> +    EmitLEConstant((Value >> 32) & 0xffff, 2, CurByte, OS);
> +    EmitLEConstant(Value & 0xffffFFFF, 4, CurByte, OS);
> +  }
> +}
> ----------------
> Why can't you just emit the value returned by getBinaryCodeForInstr() directly without re-ordering the bytes?

It's easier to encode all bits of insn in BPFInstrInfo.td this way.
Right now it emits right thing for little endian only, but encoding should
be big if underlying cpu is big-endian. So more 'if' conditions will
be added here in the future.

Thanks a lot for review!

> http://reviews.llvm.org/D6494
>
>