[PATCH] [RFC PATCH] BPF backend
Tom Stellard
tom at stellard.net
Thu Dec 4 08:04:18 PST 2014
On Wed, Dec 03, 2014 at 07:18:56PM -0800, Alexei Starovoitov wrote:
> On Wed, Dec 3, 2014 at 5:25 PM, Matt Arsenault
> <Matthew.Arsenault at amd.com> wrote:
> > ================
> > Comment at: lib/Target/BPF/BPFAsmPrinter.cpp:66-67
> > @@ +65,4 @@
> > + llvm_unreachable("<unknown operand type>");
> > + O << "bug";
> > + return;
> > + }
> > ----------------
> > Should remove this dead code
>
> you mean to leave llvm_unreachable() only.
> yep. done.
>
> > ================
> > Comment at: lib/Target/BPF/BPFFrameLowering.h:26
> > @@ +25,3 @@
> > +
> > + // llvm 3.3+ defines it here
> > + void eliminateCallFramePseudoInstr(MachineFunction &MF, MachineBasicBlock &MBB,
> > ----------------
> > This comment should be removed
>
> ohh. yes. forgot this one.
> done.
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelDAGToDAG.cpp:103
> > @@ +102,3 @@
> > + if (Node->isMachineOpcode()) {
> > + DEBUG(errs() << "== "; Node->dump(CurDAG); errs() << "\n");
> > + return NULL;
> > ----------------
> > DEBUG() printing should print to dbgs(), not errs(). This should also use single quotes around '\n'
>
> that was a copy paste from MSP430 backend.
> missed the fact that it's printing it wrong...
> fixed.
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelDAGToDAG.cpp:112-113
> > @@ +111,4 @@
> > + case ISD::UNDEF: {
> > + errs() << "BUG: "; Node->dump(CurDAG); errs() << "\n";
> > + report_fatal_error("shouldn't see UNDEF during Select");
> > + break;
> > ----------------
> > I think you should delete this error. Just printing the undef node isn't really useful, and it's better to just let this fall to the generic cannot select error
>
> It actually won't fail to select.
> It will just pick some register with implicit def and use it as 'undef'.
> That's ok for all normal programs, but here the user will be confused
> why kernel is rejecting such program.
> Kernel doesn't allow any uninitialized or implicitly initialized
> registers or memory, so compiler needs to emit explicit zero init.
> Which is achieved by
> setOperationAction(ISD::UNDEF, MVT::i64, Expand);
> (which no other backend does).
> The fatal_error() above is to make sure that selector doesn't see
> such things.
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelDAGToDAG.cpp:152-157
> > @@ +151,8 @@
> > +
> > + DEBUG(errs() << "=> ");
> > + if (ResNode == NULL || ResNode == Node)
> > + DEBUG(Node->dump(CurDAG));
> > + else
> > + DEBUG(ResNode->dump(CurDAG));
> > + DEBUG(errs() << "\n");
> > + return ResNode;
> > ----------------
> > You can put all of these under a single DEBUG()
>
> done.
> was a copy-paste from msp430
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:46-49
> > @@ +45,6 @@
> > + setOperationAction(ISD::GlobalAddress, MVT::i64, Custom);
> > + /* TODO: fail them gently
> > + setOperationAction(ISD::BlockAddress, MVT::i64, Custom);
> > + setOperationAction(ISD::JumpTable, MVT::i64, Custom);
> > + setOperationAction(ISD::ConstantPool, MVT::i64, Custom);*/
> > +
> > ----------------
> > You could try using the backend error reporting that's now available. R600 uses this for calls and some other things. I've been thinking it might be good to add an Unsupported legalize action to emit these sort of errors generically
>
> sounds great. I definitely would prefer that, but
> what do you mean by 'backend error reporting' ?
>
> >
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:109
> > @@ +108,3 @@
> > + MaxStoresPerMemmove = MaxStoresPerMemmoveOptSize = 128;
> > +}
> > +
> > ----------------
> > You aren't using setBooleanContents. I'm guessing you probably want ZeroOrOneBooleanContent rather than the default UndefinedBooleanContent.
>
> yeah. definitely. when I started that function didn't exist.
> fixed.
>
> > Also since there isn' a select, you might want to look into SelectIsExpensive
>
> hmm. good point. let me play with it.
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:160-166
> > @@ +159,9 @@
> > + default:
> > + {
> > +#ifndef NDEBUG
> > + errs() << "LowerFormalArguments Unhandled argument type: "
> > + << RegVT.getSimpleVT().SimpleTy << "\n";
> > +#endif
> > + llvm_unreachable(0);
> > + }
> > + case MVT::i64:
> > ----------------
> > This should probably just be llvm_unreachable with a message
>
> llvm_unreachable cannot print types.
> here I want to tell user which struct/union is causing compilation failure.
> In general I would love to see some generic error reporting mechanism
> with original line numbers, but I don't think one exists.
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:189-191
> > @@ +188,5 @@
> > + assert(VA.isMemLoc());
> > + errs() << "Function: " << MF.getName() << " ";
> > + MF.getFunction()->getFunctionType()->dump();
> > + errs() << "\n";
> > + report_fatal_error("too many function args");
> > ----------------
> > You can usually print to a stream with *FunctionType rather than separately calling dump
>
> yep. fixed.
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:473-474
> > @@ +472,4 @@
> > +
> > +SDValue BPFTargetLowering::LowerSELECT_CC(SDValue Op,
> > + SelectionDAG &DAG) const {
> > + SDValue LHS = Op.getOperand(0);
> > ----------------
> > I think you can get away without custom lowering select_cc, and just select from the generic select_cc node, since you just select to a pseudo anyway and have to custom inserter.
>
> yes. because 'if_cond_then_goto' is a single instruction and
> 'cond' part cannot do < and <=
> I tried several ways without custom inserter, but couldn't make it work.
> What do you mean more specifically?
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:484
> > @@ +483,3 @@
> > +
> > + SDValue TargetCC = DAG.getConstant(CC, MVT::i64);
> > +
> > ----------------
> > This should be a getTargetConstant
>
> that won't work as-is, because it matches how pseudo selectcc is defined.
> I can adjust both I guess...
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:487-492
> > @@ +486,8 @@
> > + SDVTList VTs = DAG.getVTList(Op.getValueType(), MVT::Glue);
> > + SmallVector<SDValue, 5> Ops;
> > + Ops.push_back(LHS);
> > + Ops.push_back(RHS);
> > + Ops.push_back(TargetCC);
> > + Ops.push_back(TrueV);
> > + Ops.push_back(FalseV);
> > +
> > ----------------
> > You can use a statically sized array for this
>
> makes sense.
> done.
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:495
> > @@ +494,3 @@
> > + SDValue sel = DAG.getNode(BPFISD::SELECT_CC, dl, VTs, Ops);
> > + DEBUG(errs() << "LowerSELECT_CC:\n"; sel.dumpr(); errs() << "\n");
> > + return sel;
> > ----------------
> > This is probably a pretty noisy debug statement, and I don't think any other targets have printing like this in their Lower*. Also dump without a DAG tends to crash frequently
>
> it shows that it wasn't easy to get select_cc to work correctly :)
> Now it's not needed. Removed.
>
> > ================
> > Comment at: lib/Target/BPF/BPFISelLowering.cpp:565-568
> > @@ +564,6 @@
> > + switch (CC) {
> > + case ISD::SETGT:
> > + BuildMI(BB, dl, TII.get(BPF::JSGT_rr))
> > + .addReg(LHS).addReg(RHS).addMBB(copy1MBB);
> > + break;
> > + case ISD::SETUGT:
> > ----------------
> > The usual style is to have each .add* on a separate line
>
> I see x86 and others using .addReg(..).addImm(..).addReg(...)
> on the same line.
> I think in single line it's easier to read and spot copy paste errors,
> but I can change it if ".add* on separate lines" is a de-facto standard :)
>
> > ================
> > Comment at: lib/Target/BPF/BPFInstrInfo.cpp:140
> > @@ +139,3 @@
> > + llvm_unreachable("Unexpected conditional branch");
> > + return 0;
> > +}
> > ----------------
> > Dead code
>
> was copy pasted.
> fixed.
>
> > ================
> > Comment at: lib/Target/BPF/BPFInstrInfo.h:23-26
> > @@ +22,6 @@
> > +
> > + virtual void copyPhysReg(MachineBasicBlock &MBB,
> > + MachineBasicBlock::iterator I, DebugLoc DL,
> > + unsigned DestReg, unsigned SrcReg,
> > + bool KillSrc) const;
> > +
> > ----------------
> > This and everywhere else should use override
>
> Yes. I noticed that llvm switch to explicit 'override' everywhere
> which wasn't a requirement before. I guess I was to lazy to fix it.
> Will do. Thanks!
>
> > ================
> > Comment at: lib/Target/BPF/BPFInstrInfo.td:77
> > @@ +76,3 @@
> > + let EncoderMethod = "getMemoryOpValue";
> > + let DecoderMethod = "todo_decode_memri";
> > + let MIOperandInfo = (ops GPR, i16imm);
> > ----------------
> > It's probably better to just not set this
>
> you mean 'DecodeMethod' ?
> sure. killed it.
>
> > ================
> > Comment at: lib/Target/BPF/BPFRegisterInfo.cpp:33-34
> > @@ +32,4 @@
> > + BitVector Reserved(getNumRegs());
> > + Reserved.set(BPF::R10);
> > + Reserved.set(BPF::R11);
> > + return Reserved;
> > ----------------
> > A comment about why these are reserved might be helpful
>
> because R10 is read only frame pointer and R11 is pseudo stack pointer.
> I guess it may be possible to make it without R11,
> but I felt that whole codegen may be too confused not to see
> 'stack pointer' register at all.
> Will add a comment.
>
> > ================
> > Comment at: lib/Target/BPF/BPFSubtarget.h:24
> > @@ +23,3 @@
> > +#include "llvm/Target/TargetSubtargetInfo.h"
> > +#include <string>
> > +
> > ----------------
> > I don't think you need this include here
>
> good point.
> fixed.
>
> > ================
> > Comment at: lib/Target/BPF/InstPrinter/BPFInstPrinter.cpp:7
> > @@ +6,3 @@
> > +
> > +#define DEBUG_TYPE "asm-printer"
> > +#include "BPF.h"
> > ----------------
> > DEBUG_TYPE should now go below all includes
>
> ok. fixed.
>
> > ================
> > Comment at: lib/Target/BPF/InstPrinter/BPFInstPrinter.cpp:62
> > @@ +61,3 @@
> > + if (OffsetOp.isImm()) {
> > + O << OffsetOp.getImm();
> > + } else {
> > ----------------
> > You might want to use formatImm/formatDec/formatHex instead of directly writing the immediate this way.
>
> hmm. I think it's decimal by default, but I guess won't hurt.
> fixed.
>
> > ================
> > Comment at: lib/Target/BPF/InstPrinter/BPFInstPrinter.cpp:70
> > @@ +69,3 @@
> > + assert(RegOp.isReg() && "Register operand not a register");
> > + O << "(" << getRegisterName(RegOp.getReg()) << ")";
> > +}
> > ----------------
> > Single quotes around '(' ')'
>
> fixed.
>
> > ================
> > Comment at: lib/Target/BPF/MCTargetDesc/BPFAsmBackend.cpp:58-62
> > @@ +57,7 @@
> > +
> > + if (0)
> > + errs() << "<MCFixup" << " Offset:" << Fixup.getOffset() << " Value:" <<
> > + *(Fixup.getValue()) << " Kind:" << Fixup.getKind() <<
> > + " val " << Value << ">\n";
> > +
> > + if (Fixup.getKind() == FK_SecRel_4 || Fixup.getKind() == FK_SecRel_8) {
> > ----------------
> > Dead code
>
> removed.
>
> > ================
> > Comment at: lib/Target/BPF/MCTargetDesc/BPFBaseInfo.h:14
> > @@ +13,3 @@
> > +
> > +static inline unsigned getBPFRegisterNumbering(unsigned Reg) {
> > + switch(Reg) {
> > ----------------
> > You don't need this function. See MCRegisterInfo::getEncodingValue
>
> ok. will do.
>
> > ================
> > Comment at: lib/Target/BPF/MCTargetDesc/BPFELFObjectWriter.cpp:39-44
> > @@ +38,8 @@
> > + default: llvm_unreachable("invalid fixup kind!");
> > + case FK_SecRel_8:
> > + Type = ELF::R_X86_64_64;
> > + break;
> > + case FK_SecRel_4:
> > + Type = ELF::R_X86_64_PC32;
> > + break;
> > + }
> > ----------------
> > You can just return the value from here, no need for Type
>
> took me some time to figure out what you meant by 'no need for Type'... :)
> yep. removed the variable. It's indeed shorter.
>
> Thanks a lot for thorough review!!!
>
> btw is there a way to write tests that check binary encoding
> inside generated elf files?
If you just want to check encoding of instructions, you can
use llc's -show-mc-encoding flag.
-Tom
More information about the llvm-commits
mailing list