This is an archive of the discontinued LLVM Phabricator instance.

[mlir][spirv] Fix nested control flow serialization

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



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



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();
  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
      spv.Store "Function" %var, %true : i1
      spv.Branch ^bb3
      spv.Store "Function" %var, %false : i1
      spv.Branch ^bb3
    %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):
// CHECK: [[false1]](%{{.*}}: i32, %{{.*}}: i32):
  ^false1(%arg1: i32, %arg2: i32):

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

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