This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Fix nested control flow serialization
ClosedPublic

Authored by antiagainst on Dec 10 2021, 3:48 PM.

Details

Summary

If we have a spv.mlir.selection op nested in a spv.mlir.loop
op, when serializing the loop's block, we might need to jump
from the selection op's merge block, which might be different
than the immediate MLIR IR predecessor block. But we still need
to get the block argument from the MLIR IR predecessor block.

Also, if the spv.mlir.selection is in the spv.mlir.loop's
header block, we need to make sure OpLoopMerge is emitted
in the current block before start processing the nested selection
op. Otherwise we'll see the LoopMerge in the wrong SPIR-V
basic block.

Depends On D115541

Diff Detail

Event Timeline

antiagainst created this revision.Dec 10 2021, 3:48 PM
antiagainst requested review of this revision.Dec 10 2021, 3:48 PM
Hardcode84 accepted this revision.Dec 11 2021, 1:20 AM

Thanks

mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
1022

Why did you removed seq?
Also, it probably can be changed to smth like this:

OperandRange blockOperands;
if (branchCondOp.trueTarget() == block)
  blockOperands = branchCondOp.trueTargetOperands();
else if (branchCondOp.falseTarget() == block)
  blockOperands = branchCondOp.falseTargetOperands();
else
  llvm_unreachable("Invalid BranchConditionalOp");

as we know this is the same block that contains BranchConditionalOp.

Also, I have tried similar fix locally before, but it was failing on

// RUN: mlir-translate -split-input-file -test-spirv-roundtrip %s | FileCheck %s

spv.module Logical GLSL450 requires #spv.vce<v1.0, [Shader], []> {
// CHECK-LABEL: @cond_branch_nested_block
  spv.func @cond_branch_nested_block() -> () "None" {
    %true = spv.Constant true
    %false = spv.Constant false
    %zero = spv.Constant 0 : i32
    %one = spv.Constant 1 : i32
    %var = spv.Variable : !spv.ptr<i1, Function>
    spv.mlir.selection {
      spv.BranchConditional %true, ^bb1, ^bb2
    ^bb1:
      spv.Store "Function" %var, %true : i1
      spv.Branch ^bb3
    ^bb2:
      spv.Store "Function" %var, %false : i1
      spv.Branch ^bb3
    ^bb3:
      spv.mlir.merge
    }
    %cond = spv.Load "Function" %var : i1
// CHECK:   spv.BranchConditional %{{.*}}, ^[[true1:.*]](%{{.*}} : i32), ^[[false1:.*]](%{{.*}}, %{{.*}} : i32, i32)
    spv.BranchConditional %cond, ^true1(%one: i32), ^false1(%zero, %zero: i32, i32)
// CHECK: [[true1]](%{{.*}}: i32):
  ^true1(%arg0: i32):
    spv.Return
// CHECK: [[false1]](%{{.*}}: i32, %{{.*}}: i32):
  ^false1(%arg1: i32, %arg2: i32):
    spv.Return
  }
}

With spv.Load was destroyed while have uses

This revision is now accepted and ready to land.Dec 11 2021, 1:20 AM

Address comments

antiagainst added inline comments.Dec 11 2021, 11:39 AM
mlir/lib/Target/SPIRV/Serialization/Serializer.cpp
1022

Also, it probably can be changed to smth like this:

Good point! I don't know why I went that obscure way in the beginning now.. Thanks for the suggestion! :)

Also, I have tried similar fix locally before, but it was failing on

Yes this is a separate issue which I noticed too. I've a following up patch https://reviews.llvm.org/D115582 to address it.

This revision was landed with ongoing or failed builds.Dec 11 2021, 11:47 AM
This revision was automatically updated to reflect the committed changes.