Page MenuHomePhabricator

[MLIR] Added llvm.indirectbr
Needs ReviewPublic

Authored by shraiysh on Feb 20 2020, 11:05 AM.

Details

Reviewers
rriddle
ftynse
Summary

This patch adds llvm.indirect_br.
LLVM Language reference suggests that repetition of labels, although allowed, is not useful. The same behaviour is implemented here. Also, the order of labels does not matter.

Syntax:
llvm.indirect_br <address> [ label1 label2 ... ] : <type>

Example:
LLVM IR
indirectbr i8* %address, [label %bb1, label %bb2, label %bb2]
MLIR LLVM Dialect
llvm.indirect_br %arg0 [^bb1, ^bb2] : !llvm<"i8*">

Diff Detail

Event Timeline

shraiysh created this revision.Feb 20 2020, 11:05 AM
Herald added a project: Restricted Project. · View Herald Transcript
rriddle requested changes to this revision.Feb 20 2020, 11:18 AM

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

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
853

Do we need a map here? Can you just pass a ArrayRef<std::pair<>> instead?

856

nit: We can just do: result.addOperands(address).

857

This looks like C++17, LLVM/MLIR compiles with C++14.

861

*Op classes are value-typed, so they shouldn't be passed by reference or pointer.

864

nit: Can you just implement this inline with:

interleaveComma(llvm::seq<int>(0, op.getNumSuccessors()), p, [&](int i) {
  p.printSuccessorAndUseList(op, i);
});
This revision now requires changes to proceed.Feb 20 2020, 11:18 AM
shraiysh updated this revision to Diff 245710.Feb 20 2020, 11:57 AM
shraiysh marked 5 inline comments as done.

Hi @rriddle ! Thanks for the comments. I have updated the patch with more context now. Also, I have addressed all comments except one. Please let me know if you think I should change it to ArrayRef (I mentioned my reason in the inline comment).

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
853

My reason for going with map here was that with ArrayRef<std::pair<>>, duplication of labels will have to be handled here (avoid entering the same label twice). Please let me know if you still want me to use ArrayRef<std::pair<>>.

ftynse requested changes to this revision.Feb 21 2020, 4:57 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
465

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.

mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
856

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.

858

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.

866

const auto &

mlir/lib/Target/LLVMIR/ConvertFromLLVMIR.cpp
654

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).

656

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.

mlir/test/Dialect/LLVMIR/roundtrip.mlir
291

Please avoid matching on SSA names (https://mlir.llvm.org/getting_started/TestingGuide/).

293

Please drop the autogenerated "pred" comment from the test

304

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.

306

Please also add a test with blocks taking arguments.

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

mlir/test/Target/llvmir.mlir
1181

How do you intend to handle LLVM's requirement that the argument of indirectbr must be produced by blockaddress ?

This revision now requires changes to proceed.Feb 21 2020, 4:57 AM
shraiysh updated this revision to Diff 246113.Feb 23 2020, 9:42 AM
shraiysh marked 13 inline comments as done.
shraiysh retitled this revision from [MLIR] Added llvm.indirect_br to [MLIR] Added llvm.indirectbr.
shraiysh edited the summary of this revision. (Show Details)
shraiysh marked an inline comment as not done.Feb 23 2020, 9:44 AM
shraiysh added inline comments.
mlir/lib/Dialect/LLVMIR/IR/LLVMDialect.cpp
858

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.

mlir/test/Dialect/LLVMIR/roundtrip.mlir
306

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.

mlir/test/Target/llvmir.mlir
1181

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.

shraiysh marked 3 inline comments as done.Feb 23 2020, 9:51 AM
ftynse added inline comments.Feb 24 2020, 7:55 AM
mlir/test/Dialect/LLVMIR/roundtrip.mlir
304

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).

mlir/test/Target/llvmir.mlir
1181

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.

The discussion seems to have converged to allowing repeated successors with different arguments and doing the legalization as a pre-pass to translation. Let's update this diff to reflect that.

Hi @ftynse ! I will update this diff accordingly. I'll figure out how to handle BlockAddresses first.

ftynse resigned from this revision.Apr 16 2020, 1:21 AM

This looks abandoned. Ping me if it is not the case.

Hi, Yes. I'm sorry, but I'm a little busy with other commitments right now. I'll work on this later.