Page MenuHomePhabricator

[mlir] Add structural type conversions for SCF dialect.
AbandonedPublic

Authored by silvas on Oct 15 2020, 8:59 PM.

Details

Summary

A "structural" type conversion is one where the underlying ops are
completely agnostic to the actual types involved, and simply need to
update their types accordingly.

An example of this is scf.if -- the values yielded by the regions merely
need to match the result types. The scf.if op itself just needs to
update its types accordingly with how the body is modified.

See the comment in TestBufferizeStructuralOps.cpp for more details on
the issues with the current formulation here. Any help on resolving
those issues would be greatly appreciated.

Diff Detail

Event Timeline

silvas created this revision.Oct 15 2020, 8:59 PM
silvas requested review of this revision.Oct 15 2020, 8:59 PM
herhut accepted this revision.Oct 16 2020, 2:11 AM

Thanks, looks good! Please wait for other reviewers to comment.

mlir/lib/Dialect/SCF/Transforms/StructuralTypeConversions.cpp
34

Should ResultRange and friends maybe grow a setTypes method?

43

nit: why not have one continuous namespace?

mlir/test/lib/Transforms/TestBufferizeStructuralOps.cpp
26

Could this be done by the infra itself based on RegionOpBranchInterface and ReturnLikeOp? Those are sufficient to understand region data flow, so the infra would know which operands of terminators need materialization.

This revision is now accepted and ready to land.Oct 16 2020, 2:11 AM
silvas added inline comments.Oct 16 2020, 11:27 AM
mlir/lib/Dialect/SCF/Transforms/StructuralTypeConversions.cpp
43

LLVM coding standards suggest keeping "privatizing" annotations like static on functions or anoymous namespaces on classes scoped as tightly as possible: https://llvm.org/docs/CodingStandards.html#anonymous-namespaces

mlir/test/lib/Transforms/TestBufferizeStructuralOps.cpp
26

I would love that. I'll be looking more into how to solve this soon.

rriddle added inline comments.Oct 16 2020, 11:29 AM
mlir/include/mlir/Dialect/SCF/Transforms.h
16

Drive by: This header looks like it could be replaced by forward declarations.

silvas added inline comments.Oct 16 2020, 11:30 AM
mlir/include/mlir/Dialect/SCF/Transforms.h
16

Do we have a documented forward declaration policy? I find them more error-prone so I avoid them if possible.

silvas added inline comments.Oct 16 2020, 1:21 PM
mlir/lib/Dialect/SCF/Transforms/StructuralTypeConversions.cpp
34

Good idea! I did it in a separate follow-on patch for clarity: https://reviews.llvm.org/D89591

silvas updated this revision to Diff 298741.Oct 16 2020, 1:26 PM

Use forward decls

silvas marked 2 inline comments as done.Oct 16 2020, 1:26 PM

How does it work with legality: someone has to leave scf.for/scf.if illegal for the conversion to kick-in here? But we also want them legal after the conversion.
Seems like we'd want this kind of pattern to also come with their notion of legality: a "structural op" should be marked dynamically legal based on if its invariants are correctly preserved?

mlir/include/mlir/Dialect/SCF/Transforms.h
48

Can you document the public API?

mlir/lib/Dialect/SCF/Transforms/StructuralTypeConversions.cpp
21

Doc here as well?

28

s/auto/Type/

43

Here you have two classes that could be in the same anonymous namespace declaration I believe?

silvas abandoned this revision.Oct 19 2020, 6:46 PM

Abandoning this. See the improved patch in D89526

I finally figured out how to do the proper source/target type materializations to eliminate the need for a single mega-conversion pass, even for structural conversions :)

Abandoning this. See the improved patch in D89526

You're referencing the current revision here, error in copy/paste?

I think you meant to link to D89757?

I think you meant to link to D89757?

Correct