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

[River Riddle via Phabricator via llvm-commits]

rriddle requested changes to this revision.
rriddle added a comment.
This revision now requires changes to proceed.

Thanks for the patch shraiysh! I added a few comments.

Also, could you please re-upload with more context?
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface



================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:853
+    Builder *b, OperationState &result, Value address,
+    DenseMap<Block *, SmallVector<Value, 4>> successors) {
+  SmallVector<Value, 1> ops;
----------------
Do we need a map here? Can you just pass a ArrayRef<std::pair<>> instead?


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:856
+  ops.push_back(address);
+  result.addOperands(ops);
+  for (auto [block, args] : successors)
----------------
nit: We can just do: `result.addOperands(address)`.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:857
+  result.addOperands(ops);
+  for (auto [block, args] : successors)
+    result.addSuccessor(block, args);
----------------
This looks like C++17, LLVM/MLIR compiles with C++14.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:861
+
+static void printIndirectBrOp(OpAsmPrinter &p, IndirectBrOp &op) {
+  p << op.getOperationName() << ' ' << op.address() << " : "
----------------
*Op classes are value-typed, so they shouldn't be passed by reference or pointer.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:864
+    << op.address().getType() << " [";
+  p.printSuccessorsAndUseLists(op);
+  p << "]";
----------------
nit: Can you just implement this inline with:
```
interleaveComma(llvm::seq<int>(0, op.getNumSuccessors()), p, [&](int i) {
  p.printSuccessorAndUseList(op, i);
});
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74916