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

[shraiysh via Phabricator via llvm-commits]

shraiysh marked an inline comment as not done.
shraiysh added inline comments.


================
Comment at: mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp:877
+
+  do {
+    Block *dest;
----------------
ftynse wrote:
> 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.
I have shifted the type to the end of instruction.
About the block labels in brackets, please let me know if you want me to eliminate the braces. I figured square braces would be more llvm friendly.


================
Comment at: mlir/test/Dialect/LLVMIR/roundtrip.mlir:306
+  llvm.indirect_br %arg0 : !llvm<"i8*"> [^bb3, ^bb2, ^bb2]
+}
----------------
ftynse wrote:
> Please also add a test with blocks taking arguments.
> 
> Can we branch to blocks that take different arguments / different number of arguments, btw?
I think we can branch to blocks with different arguments. Different blocks can have different PHI nodes in the LLVM IR and hence there could be different number of arguments for them and indirectbr in llvm should work when that happens.


================
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
----------------
ftynse wrote:
> How do you intend to handle LLVM's requirement that the argument of `indirectbr` must be produced by `blockaddress` ?
I tried running the following code through llvm compiler, and it seems to accept it. So, I think there is no check for this in LLVM IR. I am not sure how we could check that here too. If there is a way for it, or if I misunderstood something, please let me know.

test.ll
```
define void @indirectbrTest(i8* %address, i32* %sink) #0 {

entry:
  %x = alloca i32
  store i32 0, i32* %x
  indirectbr i32* %x, [label %bb1]

bb1:
  store volatile i32 0, i32* %sink
  ret void
}
```
I compiled this using `clang++ -c test.ll`. So, it looks like there are no checks for the argument.


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

https://reviews.llvm.org/D74916