This is an archive of the discontinued LLVM Phabricator instance.

[mlir] Add a pattern to fold single- and zero-iteration scf.forall ops.
ClosedPublic

Authored by pifon2a on Mar 6 2023, 4:24 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Mar 6 2023, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 4:24 AM
pifon2a requested review of this revision.Mar 6 2023, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2023, 4:24 AM
jreiffers accepted this revision.Mar 6 2023, 4:26 AM
This revision is now accepted and ready to land.Mar 6 2023, 4:26 AM
nicolasvasilache requested changes to this revision.Mar 6 2023, 4:26 AM

let's give some time to review

This revision now requires changes to proceed.Mar 6 2023, 4:26 AM
pifon2a updated this revision to Diff 502592.Mar 6 2023, 4:40 AM

update the test.

mlir/lib/Dialect/SCF/IR/SCF.cpp
1533

How about generalizing promoteIfSingleIteration to also have an implementation for scf.forall ?
This is a common API already used for both scf.for and affine.for (with different implementations).

We can certainly wrap a pattern around and expose with populate functions but by now it is pretty well established the key functionality is better exposed via single functional-style functions.

Additionally, the promoteIfSingleIteration are in need of a refresh while we are at it: they predate the proper use of rewriter APIs and perform unsafe RAUW, this is a good opportunity to tighten things up.

1540

Please add a helper in StaticValueUtils that returns the optional number of steps:

optional<int64_t> constantTripCount(OpFoldResult lb, OpFoldResult ub, OpFoldResult step);

We could go as far as to implement that with OpFoldResult applied = makeComposedFoldedAffineApply which would capture a bunch of nontrivial cases (but would potentially build an op unless evolved to guarantee it remains in type-land).

Then your loop goes away and the code is significantly more idiomatic:

if (llvm::any_of(... [](){ return ... == 0; }))
  rewriter.replaceOp(op, op.getOutputs());
      
llvm::make_filter_range(... [](){ if (... != 1) ...; })

etc

1567

rewriter.notifyMatchFailure

1571

This seems like the part that performs the same as promoteIfSingleIteration for scf.forall.
This seems can be extracted in a well-defined API, brought closer to the one for scf.for and reused here, instead of inlining implementations.

1602

overall the pattern itself should be ~5 lines and we gain a few new reusable APIs

1609

This feels like a generally useful utility on the op itself to provide a helper that "projects out" one of the dimensions and returning an (N-1)-D version of the op.
This can then be also reused to implement other functionality (e.g. mapping to a physical thread id that currently may jump the abstraction gap a bit too quickly).

1623

This may need to be wrapped in a rewriter.startRootUpdate

pifon2a updated this revision to Diff 504449.Mar 12 2023, 8:58 AM
pifon2a marked 7 inline comments as done.

Address the comments.

mlir/lib/Dialect/SCF/IR/SCF.cpp
1533

added promoteIfSingleIteration to SCF.h instead of scf utils.h, because otherwise there is a circular dependency.

1540

I introduced the helper, but I am not sure about idiomatic part. I tried it and it was similar amount of code, but less efficient.

1609

Shall we refactor it later, when we know the exact use case?

herhut accepted this revision.Mar 21 2023, 12:58 AM
herhut added a subscriber: herhut.

Looks good conditional on small nits.

mlir/lib/Dialect/SCF/IR/SCF.cpp
540

nit: please format.

1609

This seems reasonable to me.

2756–2757

nit: Give this a better name. ParallelOpSingleOrZeroIterationDimsFolder maybe? That would be consistent with the other one.

mlir/lib/Dialect/Utils/StaticValueUtils.cpp
9

nit: is this needed?

tschuett added inline comments.
mlir/lib/Dialect/Utils/StaticValueUtils.cpp
210

You use .has_value() in several places. They are all redundant.

pifon2a updated this revision to Diff 506897.Mar 21 2023, 3:48 AM
pifon2a marked 5 inline comments as done.

Address the comments.

nicolasvasilache resigned from this revision.Mar 21 2023, 3:51 AM

thanks for picking up the slack @herhut , won't have time to look deeper atm so please go with this

This revision is now accepted and ready to land.Mar 21 2023, 3:51 AM
pifon2a updated this revision to Diff 506900.Mar 21 2023, 3:56 AM

address comments

This revision was landed with ongoing or failed builds.Mar 21 2023, 3:59 AM
This revision was automatically updated to reflect the committed changes.