[llvm] d542a56 - [BPF] Clean up SelLowering
Eduard Zingerman via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 31 14:34:47 PDT 2023
Author: Tamir Duberstein
Date: 2023-08-01T00:31:12+03:00
New Revision: d542a56c1c2cc5d14a15aa6326e5a39414cd22e2
URL: https://github.com/llvm/llvm-project/commit/d542a56c1c2cc5d14a15aa6326e5a39414cd22e2
DIFF: https://github.com/llvm/llvm-project/commit/d542a56c1c2cc5d14a15aa6326e5a39414cd22e2.diff
LOG: [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
Added:
Modified:
llvm/lib/Target/BPF/BPFISelLowering.cpp
llvm/lib/Target/BPF/BPFISelLowering.h
llvm/test/CodeGen/BPF/many_args1.ll
llvm/test/CodeGen/BPF/many_args2.ll
llvm/test/CodeGen/BPF/struct_ret1.ll
llvm/test/CodeGen/BPF/vararg1.ll
Removed:
################################################################################
diff --git a/llvm/lib/Target/BPF/BPFISelLowering.cpp b/llvm/lib/Target/BPF/BPFISelLowering.cpp
index 5e84af0095914e..ffeef14d413b47 100644
--- a/llvm/lib/Target/BPF/BPFISelLowering.cpp
+++ b/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,17 +254,18 @@ 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:
@@ -275,18 +273,22 @@ void BPFTargetLowering::ReplaceNodeResults(
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:
@@ -294,9 +296,7 @@ SDValue BPFTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
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);
}
diff --git a/llvm/lib/Target/BPF/BPFISelLowering.h b/llvm/lib/Target/BPF/BPFISelLowering.h
index e78f4d9829cb63..348510355be881 100644
--- a/llvm/lib/Target/BPF/BPFISelLowering.h
+++ b/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,
diff --git a/llvm/test/CodeGen/BPF/many_args1.ll b/llvm/test/CodeGen/BPF/many_args1.ll
index 08218f452d061d..9e9a3fdfc95344 100644
--- a/llvm/test/CodeGen/BPF/many_args1.ll
+++ b/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 {
diff --git a/llvm/test/CodeGen/BPF/many_args2.ll b/llvm/test/CodeGen/BPF/many_args2.ll
index a69886c2b20819..c5d6349dd6e1a7 100644
--- a/llvm/test/CodeGen/BPF/many_args2.ll
+++ b/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 {
diff --git a/llvm/test/CodeGen/BPF/struct_ret1.ll b/llvm/test/CodeGen/BPF/struct_ret1.ll
index 9a43cc8fc0c90a..387e466db42357 100644
--- a/llvm/test/CodeGen/BPF/struct_ret1.ll
+++ b/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
+}
diff --git a/llvm/test/CodeGen/BPF/vararg1.ll b/llvm/test/CodeGen/BPF/vararg1.ll
index 4a22db65e6928d..2b5ed16ddf135f 100644
--- a/llvm/test/CodeGen/BPF/vararg1.ll
+++ b/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 {
More information about the llvm-commits
mailing list