Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MLIR] Conversion from cf to llvm dialect fails for basic blocks with index arguments #55301

Closed
andidr opened this issue May 6, 2022 · 4 comments
Assignees

Comments

@andidr
Copy link
Contributor

andidr commented May 6, 2022

The pass cf-to-llvm fails for IR containing basic blocks with index arguments. The error occurs upon the invocation of the legalizer after the generation of cf.br ops during dialect conversion in OneToOneLLVMTerminatorLowering<cf::BranchOp, LLVM::BrOp>::matchAndRewrite() here. The cause seems to be an unrealized conversion cast that was generated as an unrealized target materialization.

The following example IR filling a single memref<2xi64> with the value 1:

module {
  func @foo(%arg0: memref<2xi64>) {
    %c0 = arith.constant 0 : index
    %c1 = arith.constant 1 : index
    %c2 = arith.constant 2 : index
    %c1_i64 = arith.constant 1 : i64
    cf.br ^bb1(%c0 : index)
  ^bb1(%0: index):  // 2 preds: ^bb0, ^bb2
    %1 = arith.cmpi slt, %0, %c2 : index
    cf.cond_br %1, ^bb2, ^bb3
  ^bb2:  // pred: ^bb1
    memref.store %c1_i64, %arg0[%0] : memref<2xi64>
    %2 = arith.addi %0, %c1 : index
    cf.br ^bb1(%2 : index)
  ^bb3:  // pred: ^bb1
    return
  }
}

triggers the error when running mlir-opt --convert-cf-to-llvm fill-memref.cf.mlir:

fill-memref.cf.mlir:7:5: error: type mismatch for bb argument #0 of successor #0
cf.br ^bb1(%c0 : index)
^
fill-memref.cf.mlir:7:5: note: see current operation: "llvm.br"(%1)[^bb1] : (i64) -> ()

The last IR dump right before exiting with an error is:

// *** IR Dump After Pattern Application ***
"func.func"() ({
^bb0(%arg0: memref<2xi64>):
  %0 = "arith.constant"() {value = 0 : index} : () -> index
  %1 = "builtin.unrealized_conversion_cast"(%0) : (index) -> i64
  %2 = "arith.constant"() {value = 1 : index} : () -> index
  %3 = "arith.constant"() {value = 2 : index} : () -> index
  %4 = "arith.constant"() {value = 1 : i64} : () -> i64
  "llvm.br"(%1)[^bb1] : (i64) -> ()
  "cf.br"(%0)[^bb1] : (index) -> ()
^bb1(%5: index):  // 4 preds: ^bb0, ^bb0, ^bb2, ^bb2
  %6 = "arith.cmpi"(%5, %3) {predicate = 2 : i64} : (index, index) -> i1
  "llvm.cond_br"(%6)[^bb2, ^bb3] {operand_segment_sizes = dense<[1, 0, 0]> : vector<3xi32>} : (i1) -> ()
  "cf.cond_br"(%6)[^bb2, ^bb3] {operand_segment_sizes = dense<[1, 0, 0]> : vector<3xi32>} : (i1) -> ()
^bb2:  // 2 preds: ^bb1, ^bb1
  "memref.store"(%4, %arg0, %5) : (i64, memref<2xi64>, index) -> ()
  %7 = "arith.addi"(%5, %2) : (index, index) -> index
  %8 = "builtin.unrealized_conversion_cast"(%7) : (index) -> i64
  "llvm.br"(%8)[^bb1] : (i64) -> ()
  "cf.br"(%7)[^bb1] : (index) -> ()
^bb3:  // 2 preds: ^bb1, ^bb1
  "func.return"() : () -> ()
}) {function_type = (memref<2xi64>) -> (), sym_name = "foo"} : () -> ()

Tested with commit a65afce.

@llvmbot
Copy link
Collaborator

llvmbot commented May 6, 2022

@llvm/issue-subscribers-mlir

@llvmbot
Copy link
Collaborator

llvmbot commented May 6, 2022

@llvm/issue-subscribers-mlir-llvm

@Berke-Ates
Copy link

For anyone who faces the same challenge, I’ve written a simple pass that can resolve the issue:
https://github.com/Berke-Ates/mlir-cf-pass

@tpopp tpopp self-assigned this Jul 21, 2022
tpopp added a commit that referenced this issue Aug 4, 2022
Previously cf.br cf.cond_br and cf.switch always lowered to their LLVM
equivalents. These ops are all ops that take in some values of given
types and jump to other blocks with argument lists of the same types. If
the types are not the same, a verification failure will later occur. This led
to confusions, as everything works when func->llvm and cf->llvm lowering
both occur because func->llvm updates the blocks and argument lists
while cf->llvm updates the branching ops. Without func->llvm though,
there will potentially be a type mismatch.

This change now only lowers the CF ops if they will later pass
verification. This is possible because the parent op and its blocks will
be updated before the contained branching ops, so they can test their
new operand types against the types of the blocks they jump to.

Another plan was to have func->llvm only update the entry block
signature and to allow cf->llvm to update all other blocks, but this had
2 problems:
1. This would create a FuncOp lowering in cf->llvm lowering which is
   awkward
2. This new pattern would only be applied if the containing FuncOp is
   marked invalid. This is infeasible with the shared LLVM type
   conversion/target infrastructure.

See previous discussions at
https://discourse.llvm.org/t/lowering-cf-to-llvm/63863 and
#55301

Differential Revision: https://reviews.llvm.org/D130971
@tpopp
Copy link
Contributor

tpopp commented Aug 4, 2022

This situation is hopefully improved with: 448adfe

Note, that this would not make your original example succeed, but it should provide insights into the reason when running with --debug and create less confusion as the ops would fail to lower to LLVM in the first place.

@tpopp tpopp closed this as completed Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants