This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][SCF] Inline ExecuteRegion if parent can contain multiple blocks
ClosedPublic

Authored by wsmoses on Jun 25 2021, 4:45 PM.

Details

Summary

The executeregionop is used to allow multiple blocks within SCF constructs. If the container allows multiple blocks, inline the region

Diff Detail

Event Timeline

wsmoses created this revision.Jun 25 2021, 4:45 PM
wsmoses requested review of this revision.Jun 25 2021, 4:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2021, 4:45 PM
wsmoses updated this revision to Diff 354630.Jun 25 2021, 4:50 PM

Add newline

bondhugula added a reviewer: bondhugula.EditedJun 25 2021, 11:37 PM
bondhugula added a subscriber: bondhugula.

The execution region op can be inlined even if its parent can only hold a single block (like scf.for, scf.if) when the execute_region only contains a single block. I think the rewrite pattern you added can be transparently generalized for that case without really any addition in code?

The more generic single block simplification you're suggesting was added here: https://reviews.llvm.org/D104865 .

The more generic single block simplification you're suggesting was added here: https://reviews.llvm.org/D104865 .

I see - thanks. I had missed that.

mlir/lib/Dialect/SCF/SCF.cpp
214

Is there an assumption here that the yield terminator will be the terminator of the last block? Is it possible that there are multiple terminators? I don't see anything in the verifier of the scf.execute_region op that prevents it.

bondhugula added inline comments.Jun 27 2021, 7:49 AM
mlir/lib/Dialect/SCF/SCF.cpp
214

scf.execute_region can have multiple blocks terminated with yield - it's no different from a function's region in that respect.

wsmoses updated this revision to Diff 354761.Jun 27 2021, 11:09 AM

Handle regions with multiple yields

ftynse added inline comments.Jun 28 2021, 1:33 AM
mlir/include/mlir/Dialect/SCF/SCFOps.td
112–113

I'm not sure it's feasible. Folders are not supposed to create new operations, and inlining most likely will (the inlined operations will be "new" in the block).

mlir/lib/Dialect/SCF/SCF.cpp
223

Nit: just use SmallVector<Value> with the default number of stack elements

227

Not: drop trivial braces

mlir/test/Dialect/SCF/canonicalize.mlir
973

Could we relax this test a little? In particular,

  • do we care that operations are on subsequent lines? (I think we don't, use CHECK)
  • do we care about type signatures of these operations? (they are not affected by the pattern, so drop them)
  • do we care about the comment // n preds:?
  • do we care about the closing brace?
  • do we care about block names that are not guaranteed to be fixed? (we don't, pattern-match them similarly to variables).

On the contrary, we do care about execute-region being removed, so I would add CHECK-NOT: execute_region.

990

Nit: could you change the name of this function to be able to tell the "test cases" apart?

This revision was not accepted when it landed; it landed in state Needs Review.Jun 28 2021, 10:10 AM
This revision was automatically updated to reflect the committed changes.

This revision was not accepted when it landed; it landed in state Needs Review.

Was this committed unintentionally? I don't see an approval from @ftynse post comments nor from anyone else.

This revision was not accepted when it landed; it landed in state Needs Review.

Was this committed unintentionally? I don't see an approval from @ftynse post comments nor from anyone else.

Oh shoot, yes completely unintentional. I'll revert now and resubmit the PR.

wsmoses reopened this revision.Jun 29 2021, 7:42 AM
wsmoses updated this revision to Diff 355278.Jun 29 2021, 9:41 AM

Reopen changes

ftynse accepted this revision.Jun 30 2021, 12:31 AM

Would you mind to mark comments as done next time? There's a checkbox in the top right corner of each comment box. This makes it easier to re-review the patch.

mlir/test/Dialect/SCF/canonicalize.mlir
977
980
1011
1014
This revision is now accepted and ready to land.Jun 30 2021, 12:31 AM
This revision was automatically updated to reflect the committed changes.
wsmoses marked 5 inline comments as done.