Page MenuHomePhabricator

scf::ForOp: Fold away iterator arguments with no use and for which the corresponding input is yielded
ClosedPublic

Authored by chelini on Mar 12 2021, 7:10 AM.

Details

Summary

Enhance 'ForOpIterArgsFolder' to remove unused iteration arguments in a
scf::ForOp. If the block argument corresponding to the given iterator has no
use and the yielded value equals the input, we fold it away.

Diff Detail

Event Timeline

chelini created this revision.Mar 12 2021, 7:10 AM
chelini requested review of this revision.Mar 12 2021, 7:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 7:10 AM

Did you consider extending the existing ForOpIterArgsFolder to capture this case ?
The existing one will drop if you yield the bbarg directly (i.e. scf.yield %1, %arg3 : i32, i32 in your example).
It should be pretty straightforward to extend the condition here:

// Forwarded is `true` when the region `iter` argument is yielded.
bool forwarded = (std::get<1>(it) == std::get<2>(it));
keepMask.push_back(!forwarded);
canonicalize |= forwarded;
if (forwarded) {

to capture the additional condition you want ?

rriddle added inline comments.Mar 12 2021, 11:46 AM
mlir/include/mlir/IR/BlockAndValueMapping.h
80 ↗(On Diff #330237)

These methods should match the traditional datastructure methods, i.e. empty()

chelini updated this revision to Diff 330683.Mar 15 2021, 9:08 AM

Updating D98503: Canonicalization for scf::ForOp

  • Use 'forOpIterArgsFolder' as suggested.

@rriddle and @nicolasvasilache thanks for the suggestions. @nicolasvasilache I updated the code as you suggested. Indeed it simplified a lot.

Did you consider extending the existing ForOpIterArgsFolder to capture this case ?
The existing one will drop if you yield the bbarg directly (i.e. scf.yield %1, %arg3 : i32, i32 in your example).
It should be pretty straightforward to extend the condition here:

// Forwarded is `true` when the region `iter` argument is yielded.
bool forwarded = (std::get<1>(it) == std::get<2>(it));
keepMask.push_back(!forwarded);
canonicalize |= forwarded;
if (forwarded) {

to capture the additional condition you want ?

chelini retitled this revision from add Canonicalization pattern 'EnableIterArgsFolderOnUnusedArgs' for scf::ForOp to scf::ForOp: Fold away iterator arguments with no use and for which the corresponding input is yielded.Mar 15 2021, 9:11 AM
chelini edited the summary of this revision. (Show Details)

Thanks!

mlir/lib/Dialect/SCF/SCF.cpp
411

Can you please also use a list here like you did below?

1) ...
2) ...

It helps see the case disjunction more easily and when we add more cases it will be grow naturally.

451

the iterator from .. -> the corresponding iter operand ?

This revision is now accepted and ready to land.Mar 15 2021, 10:04 AM
chelini updated this revision to Diff 330742.Mar 15 2021, 11:27 AM

Update comment.

chelini updated this revision to Diff 330743.Mar 15 2021, 11:31 AM
chelini marked an inline comment as done.

Update missed comment.

chelini marked an inline comment as done.Mar 15 2021, 11:33 AM

Thanks!

@nicolasvasilache I have just updated the comments. Could you please land this revision? (I don't have commit access) Thanks!