[PATCH] D156136: [BPF] Clean up SelLowering
Eduard Zingerman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Jul 28 12:41:44 PDT 2023
eddyz87 added a comment.
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.
================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:46
+ Val->print(OS);
+ OS << ' ';
+ }
----------------
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);
}
```
================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:360
}
- } else {
- fail(DL, DAG, "defined with too many args");
+ } else if (VA.isMemLoc()) {
+ HasMemArgs = true;
----------------
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.
================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:415
ISD::ArgFlagsTy Flags = Arg.Flags;
- if (!Flags.isByVal())
- continue;
-
- fail(CLI.DL, DAG, "pass by value not supported ", Callee);
+ if (Flags.isByVal()) {
+ HasByVal = true;
----------------
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;
}
```
================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:539
+ if (!VA.isRegLoc()) {
+ fail(DL, DAG, "stack returns are not supported");
+ return DAG.getNode(Opc, DL, MVT::Other, Chain);
----------------
This was an `assert`, so let's keep it as `report_fatal_error` if you want to add some context.
================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:840
// Check before we build J*_ri instruction.
- assert (isInt<32>(imm32));
+ if (!isInt<32>(imm32)) {
+ report_fatal_error("immediate overflows 32 bits: " + Twine(imm32));
----------------
Please remove braces.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156136/new/
https://reviews.llvm.org/D156136
More information about the llvm-commits
mailing list