[BPF] Clean up SelLowering

[tamird]

Summary

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.

[arsenm]

llvm/lib/Target/BPF/BPFISelLowering.cpp

41

Don't see the point of using optional with SDValue, it already acts like optional on its own

[tamird]

Remove optional.

[ast]

llvm/lib/Target/BPF/BPFISelLowering.cpp

83

Here and elsewhere.
Did you miss Eduard's comment:

• LLVM Coding Standards actually require brace-less if statements (link https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements)

[tamird]

Restored unbraced if.

llvm/lib/Target/BPF/BPFISelLowering.cpp

83

I didn't miss it. The description on those guidelines is vague and leaves much in the hands of the author. Anyway, done, I guess.

[ast]

I don't see a value in this "cleanup". Looks like code churn to me.

llvm/lib/Target/BPF/BPFISelLowering.cpp

268

and here?

384

pls drop

[tamird]

Unbrace default arms.

[tamird]

In D156136#4529021, @ast wrote:

I don't see a value in this "cleanup". Looks like code churn to me.

The value is that the current error messages are often misleading or anemic. When working on this code it's extremely painful when the only way to understand what's going on is to step through a debugger because the error messages are just "condition not met" without indicating any of the inputs.

llvm/lib/Target/BPF/BPFISelLowering.cpp

268

Done.

384

Why? This results in a static_cast because the type is not correct.

[tamird]

Fix test expected message.

[ast]

Way too many unrelated changes. Please abandon. Don't see a value in any of them.

[tamird]

In D156136#4532425, @ast wrote:

Way too many unrelated changes. Please abandon. Don't see a value in any of them.

Can you help me understand why the current shape of this code is better? My position is that this improves the quality of the code and the errors emitted by it as described in an earlier comment. It also brings this file into closer conformance with LLVM coding standards as defined in clang-tidy configs.

[tamird]

revert renaming fail -> unsupported

[tamird]

Reverted clang-tidy changes.

[tamird]

Fix typo.

[eddyz87]

I left a few minor comments.
All-in-all I think these modifications are fine if it makes BPF backend simpler to work with.
However, I'm not sure if fixing my comments is worthwhile if Alexei objects the whole idea.

llvm/lib/Target/BPF/BPFISelLowering.cpp

46

This prints extra space at the end of the output. I think it should be like this:

if (Val) {
  raw_string_ostream OS(Str);
  OS << ' ';
  Val->print(OS);
}

367–370

This changes semantics a bit, original version has else w/o condition. Looks like the third alternative is VA.isPendingLoc(). I'm not sure if this alternative is possible at this stage, but since we are reporting errors I'd prefer to have "catch-all" else branch.

425–426

I'd say that this change might be smaller:

for (auto &Arg : Outs) {
  ISD::ArgFlagsTy Flags = Arg.Flags;
  if (!Flags.isByVal())
    continue;

  fail(CLI.DL, DAG, "pass by value not supported ", Callee);
  break;
}

545

This was an assert, so let's keep it as report_fatal_error if you want to add some context.

848–849

Please remove braces.

[tamird]

@eddz87 comments.

llvm/lib/Target/BPF/BPFISelLowering.cpp

46

I don't think so; the caller has passed in a message, which we concat on line 50. We need this space so that the user's message doesn't blend in with ours.

367–370

Done.

425–426

Done.

545

Done.

848–849

Done.

[eddyz87]

llvm/lib/Target/BPF/BPFISelLowering.cpp

46

I mean, I checked it before commenting.
If you add, say '|', at the beginning and at the end of OS you'll get output like this:

test3.c:3:10: error: too many arguments|t1: i64 = GlobalAddress<ptr @foo> 0, test3.c:3:10 |
    3 |   return foo(1, 2, 3, 4, 5, 6, 7);
      |          ^
1 error generated.

Note that there is no space after "arguments" and there is an extra space before last |. This is because we are doing Msg.concat(Str), not Str.concat(Msg).

Thanks for addressing the rest of the comments.

[tamird]

Fix diagnostics whitespace.

llvm/lib/Target/BPF/BPFISelLowering.cpp

46

Thanks for explaining! Fixed and tightened up the tests.

[ast]

In D156136#4543417, @eddyz87 wrote:

I left a few minor comments.
All-in-all I think these modifications are fine if it makes BPF backend simpler to work with.
However, I'm not sure if fixing my comments is worthwhile if Alexei objects the whole idea.

Looks fine to me as well.
@yonghong-song please take a look too.

[yonghong-song]

LGTM except one minor suggestion.

llvm/lib/Target/BPF/BPFISelLowering.cpp

379

User may not understand what 'sret' means without looking at source code.
Maybe 'functions with returning struct are not supported'?

[tamird]

llvm/lib/Target/BPF/BPFISelLowering.cpp

379

Thanks, I agree this error message is not very clear. I'd like to update it, but I notice that the existing tests do not cover it. Do you know how to write a function that would produce IR with the sret attribute?

[eddyz87]

llvm/lib/Target/BPF/BPFISelLowering.cpp

379

The following C code sample:

struct bar { long a[100]; };
struct bar foo(void) {
  struct bar b;
  return b;
}

Produces IR that triggers the error:

$ clang -mcpu=v4 -O2 --target=bpf -S test3.c -emit-llvm -o -
...
%struct.bar = type { [100 x i64] }

; Function Attrs: mustprogress nofree norecurse nosync nounwind willreturn memory(none)
define dso_local void @foo(ptr noalias nocapture sret(%struct.bar) align 8 %agg.result) local_unnamed_addr #0 {
entry:
  ret void
}
...
$ clang -mcpu=v4 -O2 --target=bpf -S test3.c -emit-llvm -o - | llc
...
error: <unknown>:0:0: in function foo void (ptr): functions with VarArgs or StructRet are not supported
...

[tamird]

Improve sret message, added test

llvm/lib/Target/BPF/BPFISelLowering.cpp

379

Thanks! Done.

[yonghong-song]

The error message is not what I envisioned.

$ cat t.c
struct t {
   int a;
};
struct t foo(int a, int b) {
  struct t tmp;
  tmp.a = a + b;
  return tmp;
}
$ clang --target=bpf -O2 -g -c t.c
t.c:4:10: error: functions returning through a pointer parameter are not supported
    4 | struct t foo(int a, int b) {
      |          ^
1 error generated.
$

From source code perspective, there is no 'returning through a pointer parameter'.
The source code shows 'returning a struct'.
The error message can be 'functions returning a struct are not supported'.

'union' has the same issue,

$ cat t.c
union t {
   int a;
   int b;
};
union t foo(int a, int b) {
  union t tmp;
  tmp.a = a + b;
  return tmp;
}
$ clang --target=bpf -O2 -g -c t.c
t.c:5:9: error: functions returning through a pointer parameter are not supported
    5 | union t foo(int a, int b) {
      |         ^
1 error generated.
$

In llvm IR, 'union' is represented as 'struct' as well.

If you really want, the error message can be
functions returning a struct/union are not supported.

[tamird]

Use the same diagnostic for both aggregate return cases.

[tamird]

In D156136#4545381, @yonghong-song wrote:

The error message is not what I envisioned.

$ cat t.c
struct t {
   int a;
};
struct t foo(int a, int b) {
  struct t tmp;
  tmp.a = a + b;
  return tmp;
}
$ clang --target=bpf -O2 -g -c t.c
t.c:4:10: error: functions returning through a pointer parameter are not supported
    4 | struct t foo(int a, int b) {
      |          ^
1 error generated.
$

From source code perspective, there is no 'returning through a pointer parameter'.
The source code shows 'returning a struct'.
The error message can be 'functions returning a struct are not supported'.

'union' has the same issue,

$ cat t.c
union t {
   int a;
   int b;
};
union t foo(int a, int b) {
  union t tmp;
  tmp.a = a + b;
  return tmp;
}
$ clang --target=bpf -O2 -g -c t.c
t.c:5:9: error: functions returning through a pointer parameter are not supported
    5 | union t foo(int a, int b) {
      |         ^
1 error generated.
$

In llvm IR, 'union' is represented as 'struct' as well.

If you really want, the error message can be
functions returning a struct/union are not supported.

Good point; I changed this wording to match the return-struct-in-registers wording. I was trying to distinguish these messages, but there's no need, they are the same from the user's perspective.

[yonghong-song]

Okay, the new error message aggregate returns are not supported looks good to me. thanks!

[tamird]

Could someone land this for me please?

[eddyz87]

In D156136#4545957, @tamird wrote:

Could someone land this for me please?

I can, will do it a bit later today.

[chapuni]

llvm/test/CodeGen/BPF/many_args1.ll

3

This doesn't match in -Asserts.

error: <unknown>:0:0: in function foo i32 (i32, i32, i32): 0x55cf2d9bbd90: i64 = GlobalAddress<ptr @bar> 0 too many arguments

[tamird]

llvm/test/CodeGen/BPF/many_args1.ll

3

Sorry about that. I haven't been able to reproduce the failure. What cmake flags are you seeing the failure with?

[chapuni]

llvm/test/CodeGen/BPF/many_args1.ll

3

Have you tried CMAKE_ENABLE_ASSERTIONS=OFF?

[tamird]

llvm/test/CodeGen/BPF/many_args1.ll

3

Thanks, I thought it defaulted to off. D156769.

llvm/lib/Target/BPF/BPFISelLowering.h

private:
   // Lower the result values of a call, copying them out of physregs into vregs
   SDValue LowerCallResult(SDValue Chain, SDValue InGlue,
                           CallingConv::ID CallConv, bool IsVarArg,
                           const SmallVectorImpl<ISD::InputArg> &Ins,
                           const SDLoc &DL, SelectionDAG &DAG,
                           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,
                     SmallVectorImpl<SDValue> &InVals) const override;
 
   // Lower incoming arguments, copy physregs into vregs
   SDValue LowerFormalArguments(SDValue Chain, CallingConv::ID CallConv,
                                bool IsVarArg,

llvm/lib/Target/BPF/BPFISelLowering.cpp

 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
 #include "llvm/CodeGen/ValueTypes.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #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"
 
 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;
+  if (Val) {
     raw_string_ostream OS(Str);
-  OS << Msg;
     Val->print(OS);
-  OS.flush();
-  DAG.getContext()->diagnose(
-      DiagnosticInfoUnsupported(MF.getFunction(), Str, DL.getDebugLoc()));
+    OS << ' ';
+  }
+  MachineFunction &MF = DAG.getMachineFunction();
+  DAG.getContext()->diagnose(DiagnosticInfoUnsupported(
+      MF.getFunction(), Twine(Str).concat(Msg), DL.getDebugLoc()));
 }
 
 BPFTargetLowering::BPFTargetLowering(const TargetMachine &TM,
                                      const BPFSubtarget &STI)
     : TargetLowering(TM) {
 
   // Set up the register classes.
   addRegisterClass(MVT::i64, &BPF::GPRRegClass);
BPFTargetLowering::BPFTargetLowering(const TargetMachine &TM,
   setOperationAction(ISD::STACKSAVE, MVT::Other, Expand);
   setOperationAction(ISD::STACKRESTORE, MVT::Other, Expand);
 
   // Set unsupported atomic operations as Custom so
   // we can emit better error messages than fatal error
   // from selectiondag.
   for (auto VT : {MVT::i8, MVT::i16, MVT::i32}) {
     if (VT == MVT::i32) {
       if (STI.getHasAlu32())
         continue;
     } else {
       setOperationAction(ISD::ATOMIC_LOAD_ADD, VT, Custom);
     }
 
     setOperationAction(ISD::ATOMIC_LOAD_AND, VT, Custom);
     setOperationAction(ISD::ATOMIC_LOAD_OR, VT, Custom);
     setOperationAction(ISD::ATOMIC_LOAD_XOR, VT, Custom);
BPFTargetLowering::getConstraintType(StringRef Constraint) const {
 
   return TargetLowering::getConstraintType(Constraint);
 }
 
 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
       return std::make_pair(0U, &BPF::GPRRegClass);
     case 'w':
       if (HasAlu32)
         return std::make_pair(0U, &BPF::GPR32RegClass);
       break;
     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");
   }
 }
 
 // Calling Convention Implementation
 #include "BPFGenCallingConv.inc"
 
 SDValue BPFTargetLowering::LowerFormalArguments(
     SDValue Chain, CallingConv::ID CallConv, bool IsVarArg,
     const SmallVectorImpl<ISD::InputArg> &Ins, const SDLoc &DL,
     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;
   }
 
   MachineFunction &MF = DAG.getMachineFunction();
   MachineRegisterInfo &RegInfo = MF.getRegInfo();
 
   // Assign locations to all of the incoming arguments.
   SmallVector<CCValAssign, 16> ArgLocs;
   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:
         Register VReg = RegInfo.createVirtualRegister(
             SimpleTy == MVT::i64 ? &BPF::GPRRegClass : &BPF::GPR32RegClass);
         RegInfo.addLiveIn(VA.getLocReg(), VReg);
         SDValue ArgValue = DAG.getCopyFromReg(Chain, DL, VReg, RegVT);
 
         // If this is an value that has been promoted to wider types, insert an
         // assert[sz]ext to capture this, then truncate to the right size.
         if (VA.getLocInfo() == CCValAssign::SExt)
           ArgValue = DAG.getNode(ISD::AssertSext, DL, RegVT, ArgValue,
                                  DAG.getValueType(VA.getValVT()));
         else if (VA.getLocInfo() == CCValAssign::ZExt)
           ArgValue = DAG.getNode(ISD::AssertZext, DL, RegVT, ArgValue,
                                  DAG.getValueType(VA.getValVT()));
 
         if (VA.getLocInfo() != CCValAssign::Full)
           ArgValue = DAG.getNode(ISD::TRUNCATE, DL, VA.getValVT(), ArgValue);
 
         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 {
   SelectionDAG &DAG = CLI.DAG;
   auto &Outs = CLI.Outs;
   auto &OutVals = CLI.OutVals;
   auto &Ins = CLI.Ins;
   SDValue Chain = CLI.Chain;
   SDValue Callee = CLI.Callee;
   bool &IsTailCall = CLI.IsTailCall;
   CallingConv::ID CallConv = CLI.CallConv;
   bool IsVarArg = CLI.IsVarArg;
   MachineFunction &MF = DAG.getMachineFunction();
 
   // BPF target does not support tail call optimization.
   IsTailCall = false;
 
   switch (CallConv) {
   default:
-    report_fatal_error("Unsupported calling convention");
+    report_fatal_error("unsupported calling convention: " + Twine(CallConv));
   case CallingConv::Fast:
   case CallingConv::C:
     break;
   }
 
   // Analyze operands of the call, assigning locations to each operand.
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CallConv, IsVarArg, MF, ArgLocs, *DAG.getContext());
 
   CCInfo.AnalyzeCallOperands(Outs, getHasAlu32() ? CC_BPF32 : CC_BPF64);
 
   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());
   Chain = DAG.getCALLSEQ_START(Chain, NumBytes, 0, CLI.DL);
 
   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:
       Arg = DAG.getNode(ISD::SIGN_EXTEND, CLI.DL, VA.getLocVT(), Arg);
       break;
     case CCValAssign::ZExt:
       Arg = DAG.getNode(ISD::ZERO_EXTEND, CLI.DL, VA.getLocVT(), Arg);
       break;
     case CCValAssign::AExt:
       Arg = DAG.getNode(ISD::ANY_EXTEND, CLI.DL, VA.getLocVT(), Arg);
       break;
     }
 
     // Push arguments into RegsToPass vector
     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;
 
   // Build a sequence of copy-to-reg nodes chained together with token chain and
   // flag operands which copy the outgoing args into registers.  The InGlue in
   // necessary since all emitted instructions must be stuck together.
   for (auto &Reg : RegsToPass) {
     Chain = DAG.getCopyToReg(Chain, CLI.DL, Reg.first, Reg.second, InGlue);
     InGlue = Chain.getValue(1);
   }
 
   // If the callee is a GlobalAddress node (quite common, every direct call is)
   // turn it into a TargetGlobalAddress node so that legalize doesn't hack it.
   // Likewise ExternalSymbol -> TargetExternalSymbol.
   if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
     Callee = DAG.getTargetGlobalAddress(G->getGlobal(), CLI.DL, PtrVT,
                                         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.
   SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
   SmallVector<SDValue, 8> Ops;
   Ops.push_back(Chain);
   Ops.push_back(Callee);
 
BPFTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CallConv,
   // CCValAssign - represent the assignment of the return value to a location
   SmallVector<CCValAssign, 16> RVLocs;
   MachineFunction &MF = DAG.getMachineFunction();
 
   // CCState - Info about the registers and stack slot.
   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);
   }
 
   // Analize return values.
   CCInfo.AnalyzeReturn(Outs, getHasAlu32() ? RetCC_BPF32 : RetCC_BPF64);
 
   SDValue Glue;
   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);
 
     // Guarantee that all emitted copies are stuck together,
     // avoiding something bad.
     Glue = Chain.getValue(1);
     RetOps.push_back(DAG.getRegister(VA.getLocReg(), VA.getLocVT()));
   }
SDValue BPFTargetLowering::LowerCallResult(
     const SmallVectorImpl<ISD::InputArg> &Ins, const SDLoc &DL,
     SelectionDAG &DAG, SmallVectorImpl<SDValue> &InVals) const {
 
   MachineFunction &MF = DAG.getMachineFunction();
   // Assign locations to each value returned by this call.
   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);
   }
 
   CCInfo.AnalyzeCallResult(Ins, getHasAlu32() ? RetCC_BPF32 : RetCC_BPF64);
 
   // Copy all of the result registers out of their specified physreg.
   for (auto &Val : RVLocs) {
     Chain = DAG.getCopyFromReg(Chain, DL, Val.getLocReg(),
const char *BPFTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case BPFISD::MEMCPY:
     return "BPFISD::MEMCPY";
   }
   return nullptr;
 }
 
 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();
   SDValue GA = DAG.getTargetGlobalAddress(GV, DL, MVT::i64);
 
   return DAG.getNode(BPFISD::Wrapper, DL, MVT::i64, GA);
 }
 
BPFTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
   bool isMemcpyOp = Opc == BPF::MEMCPY;
 
 #ifndef NDEBUG
   bool isSelectRIOp = (Opc == BPF::Select_Ri ||
                        Opc == BPF::Select_Ri_64_32 ||
                        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)
     return EmitInstrWithCustomInserterMemcpy(MI, BB);
 
   bool is32BitCmp = (Opc == BPF::Select_32 ||
                      Opc == BPF::Select_32_64 ||
                      Opc == BPF::Select_Ri_32 ||
if (isSelectRROp) {
 
     if (is32BitCmp && !HasJmp32)
       RHS = EmitSubregExt(MI, BB, RHS, isSignedCmp);
 
     BuildMI(BB, DL, TII.get(NewCC)).addReg(LHS).addReg(RHS).addMBB(Copy1MBB);
   } 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);
   }
 
   // Copy0MBB:
   //  %FalseValue = ...
   //  # fallthrough to Copy1MBB
   BB = Copy0MBB;

llvm/test/CodeGen/BPF/many_args1.ll

 ; 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 {
 entry:
   %call = tail call i32 @bar(i32 %a, i32 %b, i32 %c, i32 1, i32 2, i32 3) #3
   ret i32 %call
 }
 
 declare i32 @bar(i32, i32, i32, i32, i32, i32) #1

llvm/test/CodeGen/BPF/many_args2.ll

 ; 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 {
 entry:
   ret i32 1
 }
 
 ; Function Attrs: nounwind readnone uwtable
 define i32 @foo(i32 %a, i32 %b, i32 %c) #0 {
 entry:
   ret i32 1
 }

llvm/test/CodeGen/BPF/struct_ret1.ll

 ; 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 }
 
 @s = common global %struct.S zeroinitializer, align 4
 
 ; Function Attrs: nounwind readonly uwtable
 define { i64, i32 } @bar(i32 %a, i32 %b, i32 %c, i32 %d, i32 %e) #0 {
 entry:
   %retval.sroa.0.0.copyload = load i64, ptr @s, align 4
   %retval.sroa.2.0.copyload = load i32, ptr getelementptr inbounds (%struct.S, ptr @s, i64 0, i32 2), align 4
   %.fca.0.insert = insertvalue { i64, i32 } undef, i64 %retval.sroa.0.0.copyload, 0
   %.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

 ; 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 {
 entry:
   ret void
 }