This is an archive of the discontinued LLVM Phabricator instance.

[mlir][scf] Add general affine.min canonicalization pattern
ClosedPublic

Authored by springerm on Aug 8 2021, 11:07 PM.

Details

Summary

This canonicalization simplifies affine.min operations inside "for loop"-like operations (e.g., scf.for and scf.parallel) based on two invariants:

  • iv >= lb
  • iv < lb + step * ((ub - lb - 1) floorDiv step) + 1

This commit adds a new pass canonicalize-scf-affine-min (instead of being a canonicalization pattern) to avoid dependencies between the Affine dialect and the SCF dialect.

Depends On D107222

Diff Detail

Event Timeline

springerm created this revision.Aug 8 2021, 11:07 PM
springerm requested review of this revision.Aug 8 2021, 11:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2021, 11:07 PM
ftynse added inline comments.Aug 10 2021, 9:19 AM
mlir/include/mlir/Dialect/SCF/Passes.td
20–27

Why do we need a separate pass for this? Can't this be done as part of regular -canonicalize? If there is a good reason, please state it in the commit description. If this is only useful for isolated testing (without other canonicalizations), please move this under test/lib.

mlir/lib/Dialect/SCF/Transforms/LoopSpecialization.cpp
418

Nit: this syntax is uncommon and hard to parse visually. Could you please move declarators without initializer to a separate line?

435

Nit: please move the lambda out of the function call.

436
452–453

Don't we want to use a constant instead of dimLb here if we know LB is constant?

504

Nit: let's move this lambda out of the call too.

520

Nit: return failure() here. If this function is ever extended, it will needlessly try matching other op kinds instead of bailing out, leading to surprising behaviors.

springerm updated this revision to Diff 365940.Aug 12 2021, 2:17 AM
springerm marked 7 inline comments as done.

address comments

springerm edited the summary of this revision. (Show Details)Aug 12 2021, 2:18 AM
springerm added inline comments.Aug 12 2021, 5:17 AM
mlir/include/mlir/Dialect/SCF/Passes.td
20–27

As far as I know, canonicalization patterns must be defined along with the ops of that dialect.

So this could be a canonicalization pattern of the SCF dialect. But then there is a dependency of the SCF dialect on the affine dialect. I think this is not good. Similarly, we probably don't want a dependency of the affine dialect on the SCF dialect.

mlir/lib/Dialect/SCF/Transforms/LoopSpecialization.cpp
452–453

Doesn't really matter. The current implementation is a bit shorter (fewer lines of code).

springerm updated this revision to Diff 366172.Aug 12 2021, 8:22 PM

update test case

bondhugula added inline comments.Aug 14 2021, 9:12 AM
mlir/include/mlir/Dialect/SCF/Passes.h
31

Missing doc comment.

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

for in backticks please.

springerm marked 2 inline comments as done.

rebase

ftynse accepted this revision.Aug 24 2021, 2:56 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/SCF/Passes.td
20–27

Ah yes, the infamous cross-dialect canonicalization problem. Could you add what you said as a comment above the pass definition?

mlir/lib/Dialect/SCF/Transforms/LoopSpecialization.cpp
463

Could we assert (if the call should never fail) or propagate errors (if there is a hypothetical future where it may) instead if ignoring the return value?

564

Let's propagate errors here and signalPassFailure if patterns couldn't apply. It's not impossible that further (failable) patterns are added above without changing the error propagation.

This revision is now accepted and ready to land.Aug 24 2021, 2:56 AM
springerm updated this revision to Diff 368324.Aug 24 2021, 5:22 AM
springerm marked 3 inline comments as done.

address comments

This revision was landed with ongoing or failed builds.Aug 24 2021, 3:39 PM
This revision was automatically updated to reflect the committed changes.