Page MenuHomePhabricator

[MLIR] Add llvm.switch
Needs RevisionPublic

Authored by uenoku on Mar 1 2020, 9:16 PM.

Details

Summary

This patch adds llvm.switch operation to LLVMDialect. I followed a syntax as LLVMIR.

Example

LLVMIR

switch i32 %val, label %otherwise [ i32 0, label %onzero
                                    i32 1, label %onone ]

MLIR

%0 = llvm.mlir.constant(0 : i32) : !llvm.i32
%1 = llvm.mlir.constant(1 : i32) : !llvm.i32
llvm.switch %val, ^otherwise [ %0, ^onzero, %1, ^onone ] : !llvm.i32

Currently, successors are not allowed to have block arguments because we can't directly translate block arguments into phi node.

^entry:
%0 = llvm.mlir.constant(0 : i32) : !llvm.i32
llvm.switch %val, ^dst(%val : !llvm.i32) [ %0, ^dst(val : !llvm.i32) ] : !llvm.i32

^dst(%ret: !llvm.i32):
llvm.return %ret : !llvm.i32

In this case, we can't construct %ret naively.

Diff Detail

Event Timeline

uenoku created this revision.Mar 1 2020, 9:16 PM

Thanks for the patch Hideto! Looks great. I added a few comments.

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
484

nit: Move the { to the end of the previous line.

486

Can we separate the default destination from the cases? e.g.
successors = (successor AnySuccessor:$defaultDest, VariadicSuccessor<AnySuccessor>:$cases);

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

This is because we need to support this in the exporter? Can we instead make the exporter fail?

832

The case values are required to be constant, so why not model as an attribute using something like DenseElementsAttr?

854

Please use /// for top-level comments.

ftynse requested changes to this revision.Mar 2 2020, 1:59 AM

Thanks for the patch!

Currently, successors are not allowed to have block arguments because we can't directly translate block arguments into phi node.

Could you elaborate on this? We have logic that translates branch arguments into phi nodes as a post-processing step after all blocks, it should be sufficient to modify getPHISourceValue to handle switch in addition to branches.

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

You are not guaranteed to have operands at all, please check before calling getOperand(0).

831

Nit: invert the condition

if (!isa_and_nonnull<LLVM::ConstantOp>(op.getOperand(idx).getDefiningOp())
  return op.emitOpError() << "...";
854

Stylistic nit: consider putting logical groups ( ... )? or ( ... )+ on the same line, even if it means you'd have to stop the previous line before 80 cols. It makes the syntax significantly more readable.

874

Nit: please format your code to fit in 80 columns (just run clang-format for best results).

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

Please use LLVM::SwitchOp::build instead of this low-level API. Feel free to provide more convenient ::build functions than those auto-generated by Tablegen.

I am aware that the code around this uses OperationState, but it should not be either. Unfortunately, this code was landed without giving me a chance to do a proper review.

mlir/lib/Target/LLVMIR/ModuleTranslation.cpp
370

Nit: for (unsigned i = 1, e = switchOp.getNumSuccessors(); i < e; ++i) to avoid calling the function in the loop.

This revision now requires changes to proceed.Mar 2 2020, 1:59 AM
uenoku added a comment.Mar 2 2020, 2:41 AM

@rriddle @ftynse Thank you for the comments :)

Thanks for the patch!

Currently, successors are not allowed to have block arguments because we can't directly translate block arguments into phi node.

Could you elaborate on this? We have logic that translates branch arguments into phi nodes as a post-processing step after all blocks, it should be sufficient to modify getPHISourceValue to handle switch in addition to branches.

The destinations of switch inst may not be unique so I think it is not sufficient to modify only getPHISourceValue (because getPHISouceValue assumes that all the successors are unique).

^entry:
%0 = llvm.mlir.constant(0 : i32) : !llvm.i32
llvm.switch %val, ^dst(%val : !llvm.i32) [ %0, ^dst(%0 : !llvm.i32) ] : !llvm.i32

^dst(%ret: !llvm.i32):
llvm.return %ret : !llvm.i32

For eample, in this case, we will get %ret = phi [%val, entry] [%0, entry] if we translate it normally. Possible solutions are to destruct a switch inst into the branches before lowering, or inserting dummy basic blocks.

What do you think about it?

ftynse added a comment.Mar 2 2020, 2:55 AM

For eample, in this case, we will get %ret = phi [%val, entry] [%0, entry] if we translate it normally.

This is not valid LLVM IR, or even a PHI-based SSA form, labels in phi must be different.

Possible solutions are to destruct a switch inst into the branches before lowering, or inserting dummy basic blocks. What do you think about it?

AFAIR, we forbid identical successors with different arguments in conditional branches, motivated by LLVM IR not supporting this at all. We insert dummy blocks during std->llvm conversion because std does not have such a restriction. We can impose a similar restriction on switch in the LLVM dialect and require whoever constructs the IR to deal with it. This may be suboptimal from the usability standpoint, but so is automagically inserting dummy blocks in the translation. We've had a similar discussion in the indirectbr patch, inconclusive as far as I can tell. Let's bring this to the discussion form.

I'm fine with this patch landing with your current restriction to enable progress.

uenoku marked 3 inline comments as done.EditedMar 2 2020, 4:55 AM

We can impose a similar restriction on switch in the LLVM dialect and require whoever constructs the IR to deal with it. This may be suboptimal from the usability standpoint, but so is automagically inserting dummy blocks in the translation. We've had a similar discussion in the indirectbr patch, inconclusive as far as I can tell. Let's bring this to the discussion form.

Ok, that seems fine.

See https://llvm.discourse.group/t/duplicate-successors-in-llvm-dialect/640 for repeated-successor discussion.

Thanks!

mlir/include/mlir/Dialect/LLVMIR/LLVMOps.td
486

I'll do.

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

I'd like to check here whether there are identical successors with different arguments.

832

Ok, I'll use an attribute. I just hadn't noticed I could use an attribute in such a way :)

uenoku added a comment.Mar 4 2020, 4:55 AM

As I see the discussion in the forum, it seems best to create a basic block for each case. I'll implement so.