Page MenuHomePhabricator

[MLIR][Linalg] Refactor transforms to use linalg::getDynOperands helper
ClosedPublic

Authored by rsuderman on Fri, Jan 8, 2:03 PM.

Details

Summary

getDynOperands behavior is commonly used in a number of passes. Refactored to
use a helper function and avoid code reuse.

Diff Detail

Event Timeline

rsuderman created this revision.Fri, Jan 8, 2:03 PM
rsuderman requested review of this revision.Fri, Jan 8, 2:03 PM
rriddle added inline comments.Fri, Jan 8, 2:05 PM
mlir/lib/Dialect/Linalg/Utils/Utils.cpp
372 ↗(On Diff #315512)

This looks like something that would go wherever DimOp is defined. Adding here creates an otherwise unnecessary dependency on Linalg in several places.

mehdi_amini added inline comments.Fri, Jan 8, 4:12 PM
mlir/lib/Dialect/Linalg/Utils/Utils.cpp
372 ↗(On Diff #315512)

Either we need to add a Utils.h in StandardDialect or we could make it a static method on the DimOp class itself?

rsuderman updated this revision to Diff 315554.Fri, Jan 8, 4:53 PM

Moved from Linalg/Utils to StandardOps/Utils

rriddle added inline comments.Fri, Jan 8, 4:55 PM
mlir/include/mlir/Dialect/StandardOps/Utils/Utils.h
20

This doesn't look necessary. Can you trim the scope of this include?

mlir/lib/Dialect/StandardOps/Utils/Utils.cpp
25

nit: Remove the trivial braces here.

mlir/lib/Transforms/BufferDeallocation.cpp
56

Is this necessary?

mlir/lib/Transforms/PipelineDataTransfer.cpp
20

Is this necessary?

rsuderman updated this revision to Diff 315557.Fri, Jan 8, 5:20 PM

River's comments for pairing down deps.

rsuderman marked 4 inline comments as done.Fri, Jan 8, 5:21 PM
mehdi_amini accepted this revision.Fri, Jan 8, 5:26 PM

LGTM, Thanks!

This revision is now accepted and ready to land.Fri, Jan 8, 5:26 PM

LGTM, Thanks!

mlir/include/mlir/Dialect/StandardOps/Utils/Utils.h
18

The clang-tidy warning is because you're missing a UTILS_ for the directory

rsuderman updated this revision to Diff 315957.Mon, Jan 11, 3:57 PM

Utils header guard change.