Page MenuHomePhabricator

[mlir:Transforms] Move out the remaining non-dialect independent transforms and utilities
ClosedPublic

Authored by rriddle on Jan 20 2022, 5:35 PM.

Details

Summary

This has been a major TODO for a very long time, and is necessary for establishing a proper
dialect-free dependency layering for the Transforms library. Code was moved to effectively
two main locations:

  • Affine/

There was quite a bit of affine dialect related code in Transforms/ do to historical reasons
(of a time way into MLIR's past). The following headers were moved to:
Transforms/LoopFusionUtils.h -> Dialect/Affine/LoopFusionUtils.h
Transforms/LoopUtils.h -> Dialect/Affine/LoopUtils.h
Transforms/Utils.h -> Dialect/Affine/Utils.h

The following transforms were also moved:
AffineLoopFusion, AffinePipelineDataTransfer, LoopCoalescing

  • SCF/

Only one SCF pass was in Transforms/ (likely accidentally placed here): ParallelLoopCollapsing
The SCF specific utilities in LoopUtils have been moved to SCF/Utils.h

  • Misc:

mlir::moveLoopInvariantCode was also moved to LoopLikeInterface.h given
that it is a simple utility defined in terms of LoopLikeOpInterface.

Diff Detail

Event Timeline

rriddle created this revision.Jan 20 2022, 5:35 PM
rriddle requested review of this revision.Jan 20 2022, 5:35 PM

I'm fine with all this of course, but this is moving a lot of affine stuff so if @bondhugula could have a look that'd be great ;)

bondhugula requested changes to this revision.Jan 23 2022, 1:27 AM
bondhugula added inline comments.
mlir/lib/Conversion/SCFToStandard/SCFToStandard.cpp
16

SCF to Std shouldn't be depending on affine utilities. Which util is it depending on?

mlir/lib/Dialect/Affine/Utils/LoopUtils.cpp
1832

This file is not just affine loop utils, but affine + scf loop utils. This is also why these were left in lib/Transforms/. If these have to be moved out, it should be to a third place in the tree for "multi-dialect" transform utilities. AffineUtils/Transforms and SCFUtils/Transforms have no layering relationship -- however, they share code (like for unrolling).

This revision now requires changes to proceed.Jan 23 2022, 1:27 AM
rriddle requested review of this revision.Jan 23 2022, 2:20 AM
rriddle updated this revision to Diff 402309.
rriddle edited the summary of this revision. (Show Details)
rriddle marked 2 inline comments as done.
rriddle added inline comments.Jan 23 2022, 2:21 AM
mlir/lib/Conversion/SCFToStandard/SCFToStandard.cpp
16

Seems like nothing, removed the include.

mlir/lib/Dialect/Affine/Utils/LoopUtils.cpp
1832

I'm not really sure that makes sense. In the latest diff I've pulled out most of the SCF specific utils(except this one which explicitly generates affine operations, of which SCF should not depend on affine in any way IMO), and dropped a few more unnecessary includes and this file is effectively only used by affine dialect transformations. If there are specific utilities that don't depend on affine, we can pull them out; but at this point this file is not general enough to warrant living anywhere else IMHO. If we want the same utility implementation for Affine loops and SCF loops, I'd rather explore interface opportunities than add a new library to bless this existing code (which should really get a cleanup).

bondhugula added inline comments.Jan 24 2022, 7:46 AM
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
22

std -> llvm conversion shouldn't need anything from affine utils. This can be deleted.

rriddle updated this revision to Diff 402581.Jan 24 2022, 10:07 AM
rriddle marked an inline comment as done.Jan 24 2022, 10:08 AM
rriddle added inline comments.
mlir/lib/Conversion/StandardToLLVM/StandardToLLVM.cpp
22

Removed (and also removed a bunch of other unnecessary uses of this include).

bondhugula accepted this revision.Jan 24 2022, 6:11 PM

LGTM - thanks!

This revision is now accepted and ready to land.Jan 24 2022, 6:11 PM
This revision was landed with ongoing or failed builds.Jan 24 2022, 7:30 PM
This revision was automatically updated to reflect the committed changes.
rriddle marked an inline comment as done.