Page MenuHomePhabricator

[mlir] spirv: Add scf.while spirv conversion
ClosedPublic

Authored by Hardcode84 on Nov 2 2021, 4:56 AM.

Details

Summary
  • It works similar to scf.for coversion, but convert condition and yield ops as part of scf.whille pattern so it don't need to maintain external state

Diff Detail

Event Timeline

Hardcode84 created this revision.Nov 2 2021, 4:56 AM
Hardcode84 requested review of this revision.Nov 2 2021, 4:56 AM
antiagainst requested changes to this revision.Nov 2 2021, 11:26 AM

Awesome! Yes this is a major missing pattern. I was thinking to add it by myself they other day. Thanks for handling it! Sorry I've quite a few nits, given this is control flow. Mostly around doc though. :)

mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
436

Nit: I'd prefer to just name them as beforeRegion and afterRegion for clarity. It's not quite worth saving those extra keystrokes esp. nowadays autocompletion is so good. :)

442

Please add some comment so later it's easier to understand/modify the logic here:

// Move the while before block as the initial loop header block.

445

// Move the while after block as the initial loop body block.

448

// Jump from the loop entry block to the loop header block.

453

Could you explain why we maintain the translation of result values inside this pattern by itself instead of using the common utilities? Something like

// For other SCF ops, the scf.yield op yields the value for the whole SCF op. So we use the scf.yield op as the anchor to create/load/store SPIR-V local variables. But for the scf.while op, the scf.yield op yields a value for the before region, which may not matching the whole op's result. Instead, the scf.condition op returns values matching the whole op's results. So we need to create/load/store variables according to that.

461

// Create local variables before the scf.while op.

466

// Load the final result values after the scf.while op.

470

// Store the current iteration's result value.

479

// Convert the scf.yield op to a branch back to the header block.

mlir/test/Conversion/SCFToSPIRV/while.mlir
39

Could you add an example where the before and after regions take different types of args? You can see the third example in the doc: https://mlir.llvm.org/docs/Dialects/SCFDialect/#scfwhile-mlirscfwhileop.

This revision now requires changes to proceed.Nov 2 2021, 11:26 AM

Side question: is there any particular reason why ForOp and IfOp patterns handle yieldOp via separate pattern? It seems to complicate things as it need to maintain external state (also, I am not 100% sure my handling of yieldOp and condOp is correct wrt dialect conversion).

Also, can we remove existing ForOp lowering and use scf-for-to-while pass instead (so less code to maintain) after this is merged?

Hardcode84 updated this revision to Diff 384456.Nov 3 2021, 8:29 AM

adress review comments

Hardcode84 marked 10 inline comments as done.Nov 3 2021, 8:31 AM
antiagainst accepted this revision.Nov 11 2021, 11:28 AM

Side question: is there any particular reason why ForOp and IfOp patterns handle yieldOp via separate pattern? It seems to complicate things as it need to maintain external state (also, I am not 100% sure my handling of yieldOp and condOp is correct wrt dialect conversion).

Ah actually good point; sorry I neglected this particular part at the beginning. You are right this will be problematic with dialect conversion. The way it works in dialect conversion is that dialect conversion handles op conversion from top to bottom, from external regions to internal regions. In the process the framework will try to automatically handle type conversion for operands (including using unrealized_conversion_cast if allowed by the type converter) and give access to the type converted operands in patterns via OpAdaptor. This whole process does not materialize immediately for each op; instead it "caches" various conversion steps internally and apply once at the very end if everything turns out to be fine. What we have here will intervene the framework because we handle condition ops and yield ops when still in the while op: that's not expected by the framework. It will cause issues when we need to do special handling for those condition/yield ops too (e.g., if the integer type in condition/yield is not supported by the target environment then we need to emulate it or just abort conversion. With the current impl I think that is not properly handled. I think if you remove the Int64 in the capability list in your test you'll see errors.)

Though the above just describes what a normal dialect conversion flow works. In SCF -> SPIR-V specifically, there are additional trickiness, as commented at L64-L77 in the cpp file. I won't repeat here; you can read there.

In general this kind of tightly-coupled-ops-with-regions are hard and tricky to handle. I think we need to handle the scf.condition and scf.yield in the same way it is handled right now for scf.for ops.

It would be great if you can give it a go to fix. But it's also entirely fine if you'd like to leave it to me given that these are all tricky stuff. So I'll accept this and feel free to land. We can iterate on it.

Also, can we remove existing ForOp lowering and use scf-for-to-while pass instead (so less code to maintain) after this is merged?

Let's take it step by step. The existing scf.for conversion is quite load bearing. We first need to have the scf.while op well supported and then see how to migrate over.

This revision is now accepted and ready to land.Nov 11 2021, 11:28 AM

Can it be solved by splitting SCF conversion into 2 steps:

  1. convert types of scf.for, while, yield and cond using DialectConversion, without converting operations itself
  2. Convert them using simple RewritePattern + greedyRewriter (all types were already successfully converted at this stage)

?

rriddle added a comment.EditedNov 11 2021, 1:34 PM

Side question: is there any particular reason why ForOp and IfOp patterns handle yieldOp via separate pattern? It seems to complicate things as it need to maintain external state (also, I am not 100% sure my handling of yieldOp and condOp is correct wrt dialect conversion).

Ah actually good point; sorry I neglected this particular part at the beginning. You are right this will be problematic with dialect conversion. The way it works in dialect conversion is that dialect conversion handles op conversion from top to bottom, from external regions to internal regions. In the process the framework will try to automatically handle type conversion for operands (including using unrealized_conversion_cast if allowed by the type converter) and give access to the type converted operands in patterns via OpAdaptor. This whole process does not materialize immediately for each op; instead it "caches" various conversion steps internally and apply once at the very end if everything turns out to be fine. What we have here will intervene the framework because we handle condition ops and yield ops when still in the while op: that's not expected by the framework.

That is fine, you should be able to use getRemappedValues on the yield operations operands, which would insert temporary casts as necessary. getRemappedValues should be able to properly handle unconverted values given that D111620 landed.

mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
64–77

Is this comment still true, this pattern is passing the type converter into the conversion pattern.

use getRemappedValue

antiagainst accepted this revision.Nov 15 2021, 12:56 PM

Side question: is there any particular reason why ForOp and IfOp patterns handle yieldOp via separate pattern? It seems to complicate things as it need to maintain external state (also, I am not 100% sure my handling of yieldOp and condOp is correct wrt dialect conversion).

Ah actually good point; sorry I neglected this particular part at the beginning. You are right this will be problematic with dialect conversion. The way it works in dialect conversion is that dialect conversion handles op conversion from top to bottom, from external regions to internal regions. In the process the framework will try to automatically handle type conversion for operands (including using unrealized_conversion_cast if allowed by the type converter) and give access to the type converted operands in patterns via OpAdaptor. This whole process does not materialize immediately for each op; instead it "caches" various conversion steps internally and apply once at the very end if everything turns out to be fine. What we have here will intervene the framework because we handle condition ops and yield ops when still in the while op: that's not expected by the framework.

That is fine, you should be able to use getRemappedValues on the yield operations operands, which would insert temporary casts as necessary. getRemappedValues should be able to properly handle unconverted values given that D111620 landed.

Oh wow, I didn't notice that. Seems relatively new. As always, River come to the rescue!

mlir/lib/Conversion/SCFToSPIRV/SCFToSPIRV.cpp
64–77

Probably not true anymore given that I haven't seen anything broken since you changed this. I'll verify and clean up. Thanks a ton for fixing this, River!

Thanks for adding this again, @Hardcode84!

This revision was automatically updated to reflect the committed changes.