[PATCH] D156136: [BPF] Clean up SelLowering

[Tamir Duberstein via Phabricator via llvm-commits]

tamird added inline comments.


================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:46
+    Val->print(OS);
+    OS << ' ';
+  }
----------------
eddyz87 wrote:
> 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);
>   }
> ```
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.


================
Comment at: llvm/lib/Target/BPF/BPFISelLowering.cpp:360
       }
-    } else {
-      fail(DL, DAG, "defined with too many args");
+    } else if (VA.isMemLoc()) {
+      HasMemArgs = true;
----------------
eddyz87 wrote:
> 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.
Done.


================
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;
----------------
eddyz87 wrote:
> 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;
>   }
> ```
Done.


================
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);
----------------
eddyz87 wrote:
> This was an `assert`, so let's keep it as `report_fatal_error` if you want to add some context.
Done.


================
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));
----------------
eddyz87 wrote:
> Please remove braces.
Done.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D156136/new/

https://reviews.llvm.org/D156136