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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added inline comments.


================
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]
----------------
ftynse wrote:
> 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.
You seem to have removed `DenseMap` completely, instead of replacing it with `SetVector` as suggested. Now you have a different problem, which I hinted to in another comment. You can now create an indirect branch to the same block with different values `llvm.indirectbr %condtion [^bb1(%0: i32), ^bb1(%1: i32)] `, but LLVM IR has no way of expressing this directly (a PHI node can only have one "source" value per predecessor block).


================
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
----------------
shraiysh wrote:
> 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.
It does work because the value comes from the function argument and LLVM cannot reasonably know how the function will get called :)  It should complain if you allocate an `i8*` and pass it to `indirectbr`.  It was more a forward-looking question, without being able to take the address of a block, `indirectbr` has restricted applicability.


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

https://reviews.llvm.org/D74916