[PATCH] [RFC PATCH] BPF backend
Matt Arsenault
Matthew.Arsenault at amd.com
Wed Dec 3 17:25:09 PST 2014
================
Comment at: lib/Target/BPF/BPFAsmPrinter.cpp:66-67
@@ +65,4 @@
+ llvm_unreachable("<unknown operand type>");
+ O << "bug";
+ return;
+ }
----------------
Should remove this dead code
================
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
================
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'
================
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
================
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()
================
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
================
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. Also since there isn' a select, you might want to look into SelectIsExpensive
================
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
================
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
================
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.
================
Comment at: lib/Target/BPF/BPFISelLowering.cpp:484
@@ +483,3 @@
+
+ SDValue TargetCC = DAG.getConstant(CC, MVT::i64);
+
----------------
This should be a getTargetConstant
================
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
================
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
================
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
================
Comment at: lib/Target/BPF/BPFInstrInfo.cpp:140
@@ +139,3 @@
+ llvm_unreachable("Unexpected conditional branch");
+ return 0;
+}
----------------
Dead code
================
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
================
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
================
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
================
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
================
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
================
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.
================
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 '(' ')'
================
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
================
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
================
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
http://reviews.llvm.org/D6494
More information about the llvm-commits
mailing list