This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Split out a new ControlFlow dialect from Standard
ClosedPublic

Authored by rriddle on Feb 3 2022, 9:01 PM.

Details

Summary

This dialect is intended to model lower level/branch based control-flow constructs. The initial set
of operations are: AssertOp, BranchOp, CondBranchOp, SwitchOp; all split out from the current
standard dialect.

See https://discourse.llvm.org/t/standard-dialect-the-final-chapter/6061

Diff Detail

Event Timeline

rriddle created this revision.Feb 3 2022, 9:01 PM
rriddle requested review of this revision.Feb 3 2022, 9:01 PM

I split out the huge number of tests updated into D118967 to make this one easier to review, but I'll squash when actually committing.

Mogball added a subscriber: Mogball.Feb 3 2022, 9:45 PM

I bet you're happy to be unshackled from integration

mlir/include/mlir/Conversion/ControlFlowToSPIRV/ControlFlowToSPIRV.h
17

Is this the right include?

mlir/include/mlir/Conversion/SCFToControlFlow/SCFToControlFlow.h
14

Unused include?

18–19
mlir/lib/Conversion/ControlFlowToLLVM/ControlFlowToLLVM.cpp
98

Super or Base?

We've got a number of python users of these ops. Would you accept some additions to enable python bindings for the cf dialect to land with this? If you've got a branch somewhere, I can just push a commit to that.

We've got a number of python users of these ops. Would you accept some additions to enable python bindings for the cf dialect to land with this? If you've got a branch somewhere, I can just push a commit to that.

Ah, that would be nice (I know little to nothing about the python stuff). I've been pushing this to https://github.com/River707/llvm-project/tree/cleanup.

Cool - I'll send you a PR against that branch tonight or tomorrow and you can take it from there. Just boilerplate...

Here you go: https://github.com/River707/llvm-project/pull/1 (you should
just be able to merge that into your fork or pick it manually)

You had a failing Python test because of the move, and that is fixed now.

stellaraccident accepted this revision.Feb 5 2022, 11:20 PM

Generally, LGTM. A few minor comments.

mlir/docs/DialectConversion.md
118

I am so glad to have these properly namespaced now.

mlir/include/mlir/Conversion/ControlFlowToSPIRV/ControlFlowToSPIRV.h
17

Seems like we should just not include anything and forward declare RewritePatternSet (that is what ControlFlowToLLVM does)

mlir/include/mlir/Conversion/Passes.td
188

Does this really need to be a Module-pass? The Standard->* passes look like they were all Module-passes, but I wonder how much that is legacy and doesn't apply to a more focused dialect.

mlir/lib/Target/Cpp/TranslateToCpp.cpp
25

Isn't this supposed to be sorted to a come first in a block by itself?

This revision is now accepted and ready to land.Feb 5 2022, 11:20 PM
rriddle marked 8 inline comments as done.Feb 6 2022, 1:53 PM
rriddle added inline comments.
mlir/include/mlir/Conversion/ControlFlowToSPIRV/ControlFlowToSPIRV.h
17

Yep, thanks for pointing all of these out (just copied from the original file, which should also be doing the same)

mlir/include/mlir/Conversion/Passes.td
188

Yep, AssertOp may insert a function as part of the lowering (which requires updating the module).

mlir/lib/Target/Cpp/TranslateToCpp.cpp
25

No, system includes are supposed to come last:

https://llvm.org/docs/CodingStandards.html#include-style

Just a drive-by comment, I think the flang changes are missing.

rriddle marked 3 inline comments as done.Feb 6 2022, 2:29 PM

Just a drive-by comment, I think the flang changes are missing.

Yep, thanks. I have those fixups and will squash them in when landing.

This revision was landed with ongoing or failed builds.Feb 6 2022, 2:52 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald Transcript

I'm seeing the following test failure

mlir/test/Integration/GPU/CUDA/TensorCore/wmma-matmul-f16.mlir:26:3: error: type mismatch for bb argument #0 of successor #0
  scf.for %arg0 = %c0 to %c16 step %c1 {
  ^

in one build.

I'm seeing the following test failure

mlir/test/Integration/GPU/CUDA/TensorCore/wmma-matmul-f16.mlir:26:3: error: type mismatch for bb argument #0 of successor #0
  scf.for %arg0 = %c0 to %c16 step %c1 {
  ^

in one build.

Should already be fixed at head.

mlir/include/mlir/Conversion/SCFToControlFlow/SCFToControlFlow.h