[PATCH] D75314: [mlir] Add a new BranchOpInterface to allow for opaquely interfacing with branching terminator operations.

[Lei Zhang via Phabricator via llvm-commits]

antiagainst accepted this revision.
antiagainst added a comment.
This revision is now accepted and ready to land.

Cool! I just have a few nits.



================
Comment at: mlir/include/mlir/Analysis/ControlFlowInterfaces.h:24
+/// Erase an operand from a branch operation that is used as a successor
+/// operand. 'index' is the operand within 'operands' to be erased.
+void eraseBranchSuccessorOperand(OperandRange operands, unsigned index,
----------------
Use back tick for quotes to be consistent with others?


================
Comment at: mlir/include/mlir/Analysis/ControlFlowInterfaces.h:25
+/// operand. 'index' is the operand within 'operands' to be erased.
+void eraseBranchSuccessorOperand(OperandRange operands, unsigned index,
+                                 Operation *op);
----------------
Minor issue: I see we have a few range concepts: the whole range for all the operands and the partial range for a specific operand. We use index to mean indexing into both. What about using "offset" for the latter to differentiate?


================
Comment at: mlir/include/mlir/Analysis/ControlFlowInterfaces.td:10
+// This file contains a set of interfaces that can be used to define information
+// about what control flow operations, e.g. branches.
+//
----------------
remove "what".


================
Comment at: mlir/include/mlir/Dialect/SPIRV/SPIRVControlFlowOps.td:24
+def SPV_BranchOp : SPV_Op<"Branch",[
+    InFunctionScope, Terminator,
+    DeclareOpInterfaceMethods<BranchOpInterface>]> {
----------------
Nit: can you sort traits alphabetically?


================
Comment at: mlir/lib/IR/OperationSupport.cpp:188
+/// must not be empty.
+unsigned OperandRange::getBeginOperandNumber() const {
+  assert(!empty() && "range must not be empty");
----------------
Nit: Number typically makes me think about count or something. What about Index? But I see we also have getOperandNumber(). So meh. Up to you. :) 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75314