[BPF] Clean up SelLowering

This patch contains a number of uncontroversial changes:
- Replace all uses of
  `errs`, `assert`, `llvm_unreachable` with `report_fatal_error` with
  informative error strings.
- Replace calls to `fail` in loops with at most one call per error
  instance. Previously a function with 19 arguments would log "too many
  args" 14 times. This was not helpful.
- Change one `if (..) switch ...` to `if (..) { switch ...`. The added
  brace is consistent with a near-identical switch immediately above.
- Elide one `SDValue` copy by using a reference rather than value. This
  is consistent with a variable declared immediately before it.

Reviewed By: yonghong-song

Differential Revision: https://reviews.llvm.org/D156136

llvm/lib/Target/BPF/BPFISelLowering.cpp

@@ -26,7 +26,9 @@
 #include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/MathExtras.h"
 #include "llvm/Support/raw_ostream.h"
+
 using namespace llvm;
 
 #define DEBUG_TYPE "bpf-lower"
@@ -35,22 +37,17 @@ static cl::opt<bool> BPFExpandMemcpyInOrder("bpf-expand-memcpy-in-order",
   cl::Hidden, cl::init(false),
   cl::desc("Expand memcpy into load/store pairs in order"));
 
-static void fail(const SDLoc &DL, SelectionDAG &DAG, const Twine &Msg) {
-  MachineFunction &MF = DAG.getMachineFunction();
-  DAG.getContext()->diagnose(
-      DiagnosticInfoUnsupported(MF.getFunction(), Msg, DL.getDebugLoc()));
-}
-
-static void fail(const SDLoc &DL, SelectionDAG &DAG, const char *Msg,
-                 SDValue Val) {
-  MachineFunction &MF = DAG.getMachineFunction();
+static void fail(const SDLoc &DL, SelectionDAG &DAG, const Twine &Msg,
+                 SDValue Val = {}) {
   std::string Str;
-  raw_string_ostream OS(Str);
-  OS << Msg;
-  Val->print(OS);
-  OS.flush();
-  DAG.getContext()->diagnose(
-      DiagnosticInfoUnsupported(MF.getFunction(), Str, DL.getDebugLoc()));
+  if (Val) {
+    raw_string_ostream OS(Str);
+    Val->print(OS);
+    OS << ' ';
+  }
+  MachineFunction &MF = DAG.getMachineFunction();
+  DAG.getContext()->diagnose(DiagnosticInfoUnsupported(
+      MF.getFunction(), Twine(Str).concat(Msg), DL.getDebugLoc()));
 }
 
 BPFTargetLowering::BPFTargetLowering(const TargetMachine &TM,
@@ -245,7 +242,7 @@ std::pair<unsigned, const TargetRegisterClass *>
 BPFTargetLowering::getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                                 StringRef Constraint,
                                                 MVT VT) const {
-  if (Constraint.size() == 1)
+  if (Constraint.size() == 1) {
     // GCC Constraint Letters
     switch (Constraint[0]) {
     case 'r': // GENERAL_REGS
@@ -257,46 +254,49 @@ BPFTargetLowering::getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
     default:
       break;
     }
+  }
 
   return TargetLowering::getRegForInlineAsmConstraint(TRI, Constraint, VT);
 }
 
 void BPFTargetLowering::ReplaceNodeResults(
   SDNode *N, SmallVectorImpl<SDValue> &Results, SelectionDAG &DAG) const {
-  const char *err_msg;
+  const char *Msg;
   uint32_t Opcode = N->getOpcode();
   switch (Opcode) {
   default:
-    report_fatal_error("Unhandled custom legalization");
+    report_fatal_error("unhandled custom legalization: " + Twine(Opcode));
   case ISD::ATOMIC_LOAD_ADD:
   case ISD::ATOMIC_LOAD_AND:
   case ISD::ATOMIC_LOAD_OR:
   case ISD::ATOMIC_LOAD_XOR:
   case ISD::ATOMIC_SWAP:
   case ISD::ATOMIC_CMP_SWAP_WITH_SUCCESS:
     if (HasAlu32 || Opcode == ISD::ATOMIC_LOAD_ADD)
-      err_msg = "Unsupported atomic operations, please use 32/64 bit version";
+      Msg = "unsupported atomic operation, please use 32/64 bit version";
     else
-      err_msg = "Unsupported atomic operations, please use 64 bit version";
+      Msg = "unsupported atomic operation, please use 64 bit version";
     break;
   }
 
   SDLoc DL(N);
-  fail(DL, DAG, err_msg);
+  // We'll still produce a fatal error downstream, but this diagnostic is more
+  // user-friendly.
+  fail(DL, DAG, Msg);
 }
 
 SDValue BPFTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
   switch (Op.getOpcode()) {
+  default:
+    report_fatal_error("unimplemented opcode: " + Twine(Op.getOpcode()));
   case ISD::BR_CC:
     return LowerBR_CC(Op, DAG);
   case ISD::GlobalAddress:
     return LowerGlobalAddress(Op, DAG);
   case ISD::SELECT_CC:
     return LowerSELECT_CC(Op, DAG);
   case ISD::DYNAMIC_STACKALLOC:
-    report_fatal_error("Unsupported dynamic stack allocation");
-  default:
-    llvm_unreachable("unimplemented operand");
+    report_fatal_error("unsupported dynamic stack allocation");
   }
 }
 
@@ -309,7 +309,7 @@ SDValue BPFTargetLowering::LowerFormalArguments(
     SelectionDAG &DAG, SmallVectorImpl<SDValue> &InVals) const {
   switch (CallConv) {
   default:
-    report_fatal_error("Unsupported calling convention");
+    report_fatal_error("unimplemented calling convention: " + Twine(CallConv));
   case CallingConv::C:
   case CallingConv::Fast:
     break;
@@ -323,16 +323,22 @@ SDValue BPFTargetLowering::LowerFormalArguments(
   CCState CCInfo(CallConv, IsVarArg, MF, ArgLocs, *DAG.getContext());
   CCInfo.AnalyzeFormalArguments(Ins, getHasAlu32() ? CC_BPF32 : CC_BPF64);
 
-  for (auto &VA : ArgLocs) {
+  bool HasMemArgs = false;
+  for (size_t I = 0; I < ArgLocs.size(); ++I) {
+    auto &VA = ArgLocs[I];
+
     if (VA.isRegLoc()) {
       // Arguments passed in registers
       EVT RegVT = VA.getLocVT();
       MVT::SimpleValueType SimpleTy = RegVT.getSimpleVT().SimpleTy;
       switch (SimpleTy) {
       default: {
-        errs() << "LowerFormalArguments Unhandled argument type: "
-               << RegVT << '\n';
-        llvm_unreachable(nullptr);
+        std::string Str;
+        {
+          raw_string_ostream OS(Str);
+          RegVT.print(OS);
+        }
+        report_fatal_error("unhandled argument type: " + Twine(Str));
       }
       case MVT::i32:
       case MVT::i64:
@@ -355,22 +361,27 @@ SDValue BPFTargetLowering::LowerFormalArguments(
 
         InVals.push_back(ArgValue);
 
-	break;
+        break;
       }
     } else {
-      fail(DL, DAG, "defined with too many args");
+      if (VA.isMemLoc())
+        HasMemArgs = true;
+      else
+        report_fatal_error("unhandled argument location");
       InVals.push_back(DAG.getConstant(0, DL, VA.getLocVT()));
     }
   }
-
-  if (IsVarArg || MF.getFunction().hasStructRetAttr()) {
-    fail(DL, DAG, "functions with VarArgs or StructRet are not supported");
-  }
+  if (HasMemArgs)
+    fail(DL, DAG, "stack arguments are not supported");
+  if (IsVarArg)
+    fail(DL, DAG, "variadic functions are not supported");
+  if (MF.getFunction().hasStructRetAttr())
+    fail(DL, DAG, "aggregate returns are not supported");
 
   return Chain;
 }
 
-const unsigned BPFTargetLowering::MaxArgs = 5;
+const size_t BPFTargetLowering::MaxArgs = 5;
 
 SDValue BPFTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
                                      SmallVectorImpl<SDValue> &InVals) const {
@@ -390,7 +401,7 @@ SDValue BPFTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
 
   switch (CallConv) {
   default:
-    report_fatal_error("Unsupported calling convention");
+    report_fatal_error("unsupported calling convention: " + Twine(CallConv));
   case CallingConv::Fast:
   case CallingConv::C:
     break;
@@ -405,14 +416,14 @@ SDValue BPFTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   unsigned NumBytes = CCInfo.getStackSize();
 
   if (Outs.size() > MaxArgs)
-    fail(CLI.DL, DAG, "too many args to ", Callee);
+    fail(CLI.DL, DAG, "too many arguments", Callee);
 
   for (auto &Arg : Outs) {
     ISD::ArgFlagsTy Flags = Arg.Flags;
     if (!Flags.isByVal())
       continue;
-
-    fail(CLI.DL, DAG, "pass by value not supported ", Callee);
+    fail(CLI.DL, DAG, "pass by value not supported", Callee);
+    break;
   }
 
   auto PtrVT = getPointerTy(MF.getDataLayout());
@@ -421,16 +432,14 @@ SDValue BPFTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   SmallVector<std::pair<unsigned, SDValue>, MaxArgs> RegsToPass;
 
   // Walk arg assignments
-  for (unsigned i = 0,
-                e = std::min(static_cast<unsigned>(ArgLocs.size()), MaxArgs);
-       i != e; ++i) {
+  for (size_t i = 0; i < std::min(ArgLocs.size(), MaxArgs); ++i) {
     CCValAssign &VA = ArgLocs[i];
-    SDValue Arg = OutVals[i];
+    SDValue &Arg = OutVals[i];
 
     // Promote the value if needed.
     switch (VA.getLocInfo()) {
     default:
-      llvm_unreachable("Unknown loc info");
+      report_fatal_error("unhandled location info: " + Twine(VA.getLocInfo()));
     case CCValAssign::Full:
       break;
     case CCValAssign::SExt:
@@ -448,7 +457,7 @@ SDValue BPFTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
     if (VA.isRegLoc())
       RegsToPass.push_back(std::make_pair(VA.getLocReg(), Arg));
     else
-      llvm_unreachable("call arg pass bug");
+      report_fatal_error("stack arguments are not supported");
   }
 
   SDValue InGlue;
@@ -469,9 +478,9 @@ SDValue BPFTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
                                         G->getOffset(), 0);
   } else if (ExternalSymbolSDNode *E = dyn_cast<ExternalSymbolSDNode>(Callee)) {
     Callee = DAG.getTargetExternalSymbol(E->getSymbol(), PtrVT, 0);
-    fail(CLI.DL, DAG, Twine("A call to built-in function '"
-                            + StringRef(E->getSymbol())
-                            + "' is not supported."));
+    fail(CLI.DL, DAG,
+         Twine("A call to built-in function '" + StringRef(E->getSymbol()) +
+               "' is not supported."));
   }
 
   // Returns a chain & a flag for retval copy to use.
@@ -519,7 +528,7 @@ BPFTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CallConv,
   CCState CCInfo(CallConv, IsVarArg, MF, RVLocs, *DAG.getContext());
 
   if (MF.getFunction().getReturnType()->isAggregateType()) {
-    fail(DL, DAG, "only integer returns supported");
+    fail(DL, DAG, "aggregate returns are not supported");
     return DAG.getNode(Opc, DL, MVT::Other, Chain);
   }
 
@@ -530,9 +539,10 @@ BPFTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CallConv,
   SmallVector<SDValue, 4> RetOps(1, Chain);
 
   // Copy the result values into the output registers.
-  for (unsigned i = 0; i != RVLocs.size(); ++i) {
+  for (size_t i = 0; i != RVLocs.size(); ++i) {
     CCValAssign &VA = RVLocs[i];
-    assert(VA.isRegLoc() && "Can only return in registers!");
+    if (!VA.isRegLoc())
+      report_fatal_error("stack return values are not supported");
 
     Chain = DAG.getCopyToReg(Chain, DL, VA.getLocReg(), OutVals[i], Glue);
 
@@ -561,10 +571,10 @@ SDValue BPFTargetLowering::LowerCallResult(
   SmallVector<CCValAssign, 16> RVLocs;
   CCState CCInfo(CallConv, IsVarArg, MF, RVLocs, *DAG.getContext());
 
-  if (Ins.size() >= 2) {
+  if (Ins.size() > 1) {
     fail(DL, DAG, "only small returns supported");
-    for (unsigned i = 0, e = Ins.size(); i != e; ++i)
-      InVals.push_back(DAG.getConstant(0, DL, Ins[i].VT));
+    for (auto &In : Ins)
+      InVals.push_back(DAG.getConstant(0, DL, In.VT));
     return DAG.getCopyFromReg(Chain, DL, 1, Ins[0].VT, InGlue).getValue(1);
   }
 
@@ -650,8 +660,10 @@ const char *BPFTargetLowering::getTargetNodeName(unsigned Opcode) const {
 
 SDValue BPFTargetLowering::LowerGlobalAddress(SDValue Op,
                                               SelectionDAG &DAG) const {
-  auto N = cast<GlobalAddressSDNode>(Op);
-  assert(N->getOffset() == 0 && "Invalid offset for global address");
+  auto *N = cast<GlobalAddressSDNode>(Op);
+  if (N->getOffset() != 0)
+    report_fatal_error("invalid offset for global address: " +
+                       Twine(N->getOffset()));
 
   SDLoc DL(Op);
   const GlobalValue *GV = N->getGlobal();
@@ -742,9 +754,8 @@ BPFTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
                        Opc == BPF::Select_Ri_32 ||
                        Opc == BPF::Select_Ri_32_64);
 
-
-  assert((isSelectRROp || isSelectRIOp || isMemcpyOp) &&
-         "Unexpected instr type to insert");
+  if (!(isSelectRROp || isSelectRIOp || isMemcpyOp))
+    report_fatal_error("unhandled instruction type: " + Twine(Opc));
 #endif
 
   if (isMemcpyOp)
@@ -834,7 +845,8 @@ BPFTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
   } else {
     int64_t imm32 = MI.getOperand(2).getImm();
     // Check before we build J*_ri instruction.
-    assert (isInt<32>(imm32));
+    if (!isInt<32>(imm32))
+      report_fatal_error("immediate overflows 32 bits: " + Twine(imm32));
     BuildMI(BB, DL, TII.get(NewCC))
         .addReg(LHS).addImm(imm32).addMBB(Copy1MBB);
   }

llvm/lib/Target/BPF/BPFISelLowering.h

@@ -85,7 +85,7 @@ class BPFTargetLowering : public TargetLowering {
                           SmallVectorImpl<SDValue> &InVals) const;
 
   // Maximum number of arguments to a call
-  static const unsigned MaxArgs;
+  static const size_t MaxArgs;
 
   // Lower a call into CALLSEQ_START - BPFISD:CALL - CALLSEQ_END chain
   SDValue LowerCall(TargetLowering::CallLoweringInfo &CLI,

llvm/test/CodeGen/BPF/many_args1.ll

@@ -1,6 +1,6 @@
 ; RUN: not llc -march=bpf < %s 2> %t1
 ; RUN: FileCheck %s < %t1
-; CHECK: too many args
+; CHECK: error: <unknown>:0:0: in function foo i32 (i32, i32, i32): t10: i64 = GlobalAddress<ptr @bar> 0 too many arguments
 
 ; Function Attrs: nounwind uwtable
 define i32 @foo(i32 %a, i32 %b, i32 %c) #0 {

llvm/test/CodeGen/BPF/many_args2.ll

@@ -1,6 +1,6 @@
 ; RUN: not llc -march=bpf < %s 2> %t1
 ; RUN: FileCheck %s < %t1
-; CHECK: too many args
+; CHECK: error: <unknown>:0:0: in function bar i32 (i32, i32, i32, i32, i32, i32): stack arguments are not supported
 
 ; Function Attrs: nounwind readnone uwtable
 define i32 @bar(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e, i32 %f) #0 {

llvm/test/CodeGen/BPF/struct_ret1.ll

@@ -1,6 +1,6 @@
 ; RUN: not llc -march=bpf < %s 2> %t1
 ; RUN: FileCheck %s < %t1
-; CHECK: only integer returns
+; CHECK: error: <unknown>:0:0: in function bar { i64, i32 } (i32, i32, i32, i32, i32): aggregate returns are not supported
 
 %struct.S = type { i32, i32, i32 }
 
@@ -15,3 +15,13 @@ entry:
   %.fca.1.insert = insertvalue { i64, i32 } %.fca.0.insert, i32 %retval.sroa.2.0.copyload, 1
   ret { i64, i32 } %.fca.1.insert
 }
+
+; CHECK: error: <unknown>:0:0: in function baz void (ptr): aggregate returns are not supported
+
+%struct.B = type { [100 x i64] }
+
+; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
+define dso_local void @baz(ptr noalias nocapture sret(%struct.B) align 8 %agg.result) local_unnamed_addr #0 {
+entry:
+  ret void
+}

llvm/test/CodeGen/BPF/vararg1.ll

@@ -1,6 +1,6 @@
 ; RUN: not llc -march=bpf < %s 2> %t1
 ; RUN: FileCheck %s < %t1
-; CHECK: with VarArgs
+; CHECK: error: <unknown>:0:0: in function foo void (i32, ...): variadic functions are not supported
 
 ; Function Attrs: nounwind readnone uwtable
 define void @foo(i32 %a, ...) #0 {