[PATCH] D74916: [MLIR] Added llvm.indirect_br

[Alex Zinenko via Phabricator via llvm-commits]

ftynse requested changes to this revision.
ftynse added inline comments.
This revision now requires changes to proceed.


================
Comment at: mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td:475
 }
+def LLVM_IndirectBrOp : LLVM_TerminatorOp<"indirect_br", []> {
+  let arguments = (ins LLVM_Type:$address);
----------------
Why choose a slightly different name (`indirect_br` vs `indirectbr`)? It may be slightly more readable, but is likely to cause issues due to this discrepancy.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:875
+  // duplication of labels.
+  DenseMap<Block *, SmallVector<Value, 4>> successors;
+
----------------
If LLVM supports repeated block labels, why should we restrict it?  I'm not against it fundamentally, but I'd like a better argument then "it's not useful". For example, it creates a possibility to mention the same block with different arguments.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:877
+
+  do {
+    Block *dest;
----------------
I find it unusual to have successors after the type of the argument, have you considered otherwise?  Similarly, putting the labels in brackets looks unusual, we usually put block arguments in brackets.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:885
+
+  for (auto successor : successors)
+    result.addSuccessor(successor.getFirst(), successor.getSecond());
----------------
const auto &


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:654
+    DenseMap<Block *, SmallVector<Value, 4>> succs;
+    for (auto succ : indBr->successors()) {
+      SmallVector<Value, 4> blockArguments;
----------------
`auto *`?

Generally, I find `auto` appropriate when the type is obvious from the code (which isn't the case here) or when the type is obnoxiously long (which isn't the case here either).


================
Comment at: mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp:656
+      SmallVector<Value, 4> blockArguments;
+      if (failed(processBranchArgs(indBr, succ, blockArguments)))
+        return failure();
----------------
Nit: `operator[]` on map default-initializes the value if it is not yet in the map, so you should be able to do `failed(processBranchArgs(indBr, succ, succs[blocks[succ]])` directly and avoid a temporary. I've seen another opportunity like this above.


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:291
+  %2 = llvm.mlir.constant(0 : i32) : !llvm.i32
+  // CHECK: llvm.indirect_br %arg0 : !llvm<"i8*"> [^bb{{[12]}}, ^bb{{[12]}}]
+  llvm.indirect_br %arg0 : !llvm<"i8*"> [^bb2, ^bb1]
----------------
Please avoid matching on SSA names (https://mlir.llvm.org/getting_started/TestingGuide/).


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:293
+  llvm.indirect_br %arg0 : !llvm<"i8*"> [^bb2, ^bb1]
+^bb1:	// pred: ^bb0
+  llvm.store %2, %arg1 : !llvm<"i32*">
----------------
Please drop the autogenerated "pred" comment from the test


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:304
+^bb4:	// 3 preds: ^bb1, ^bb2, ^bb3
+  // CHECK: llvm.indirect_br %arg0 : !llvm<"i8*"> [^bb{{[23]}}, ^bb{{[23]}}]
+  llvm.indirect_br %arg0 : !llvm<"i8*"> [^bb3, ^bb2, ^bb2]
----------------
The {{[23]}} regexp is a sign that you produce non-deterministic IR (due to the order in DenseMap), which is arguably a bad thing for testability. Consider using `SetVector` or another container with a deterministic order instead.


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:306
+  llvm.indirect_br %arg0 : !llvm<"i8*"> [^bb3, ^bb2, ^bb2]
+}
----------------
Please also add a test with blocks taking arguments.

Can we branch to blocks that take different arguments / different number of arguments, btw?


================
Comment at: mlir/test/Target/llvmir.mlir:1181
+  // CHECK: indirectbr i8* %0, [label %{{[34]}}, label %{{[34]}}]
+  llvm.indirect_br %arg0 : !llvm<"i8*"> [^bb1, ^bb2]
+^bb1:	// pred: ^bb0
----------------
How do you intend to handle LLVM's requirement that the argument of `indirectbr` must be produced by `blockaddress` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74916