[PATCH] D156136: [BPF] Clean up SelLowering

[Eduard Zingerman via Phabricator via llvm-commits]

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