Page MenuHomePhabricator

[MLIR] Add lowering for affine.parallel to scf.parallel
ClosedPublic

Authored by yash on Jul 6 2020, 9:38 AM.

Details

Summary

[MLIR] adding affine parallel conversion to scf parallel in lower affine pass

Diff Detail

Event Timeline

yash created this revision.Jul 6 2020, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2020, 9:38 AM
rriddle added inline comments.Jul 6 2020, 7:16 PM
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
369

nit: expandAffineMap can fail, please check error conditions.

374

nit: Just use a for-each loop over op.steps() instead.

yash updated this revision to Diff 277355.Mon, Jul 13, 2:42 AM

Added check file.

yash updated this revision to Diff 277362.Mon, Jul 13, 3:02 AM

Minor Updates

bondhugula requested changes to this revision.Mon, Jul 13, 3:18 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
360

Nit: use backticks instead of quotes.

370–385

This is all lacking comments - please add some.

mlir/test/Conversion/AffineToStandard/lower-affine.mlir
626–630

The test cases are insufficient - please include one with non-trivial affine map bounds.

This revision now requires changes to proceed.Mon, Jul 13, 3:18 AM
yash updated this revision to Diff 277391.Mon, Jul 13, 5:14 AM

Added non trivial cases for test file

yash updated this revision to Diff 277676.Mon, Jul 13, 11:30 PM
yash marked an inline comment as done.

Minor update.

yash updated this revision to Diff 277709.Tue, Jul 14, 1:50 AM
yash marked 3 inline comments as done.

Commented and used for-each loop over op.steps()

bondhugula requested changes to this revision.Tue, Jul 14, 7:00 AM
bondhugula added inline comments.
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
377

Please avoid names like step1. Also, avoid auto here.

This revision now requires changes to proceed.Tue, Jul 14, 7:00 AM
bondhugula added inline comments.Tue, Jul 14, 7:01 AM
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
373

Still not checking if expandAffineMap returns no value, here and below.

bondhugula added inline comments.Tue, Jul 14, 7:02 AM
mlir/test/Conversion/AffineToStandard/lower-affine.mlir
675

No need to capture A5, it's unused.

Can you please update the commit title to "[MLIR] Add lowering for affine.parallel to scf.parallel"? (Updating the commit via arc won't automatically update the title here - please hit edit here to update as well.)

yash updated this revision to Diff 278118.Wed, Jul 15, 2:30 AM
yash marked 5 inline comments as done.

Checking error conditions for expandAffineMap

yash retitled this revision from [MLIR] adding affine parallel in lower affine pass to [MLIR] Add lowering for affine.parallel to scf.parallel.Wed, Jul 15, 2:34 AM
yash updated this revision to Diff 278119.Wed, Jul 15, 2:34 AM
yash retitled this revision from [MLIR] Add lowering for affine.parallel to scf.parallel to [MLIR] adding affine parallel in lower affine pass.

Changing commit title

Harbormaster completed remote builds in B64322: Diff 278118.
yash updated this revision to Diff 278134.Wed, Jul 15, 4:06 AM

Checking error conditions for expandAffineMap.

yash retitled this revision from [MLIR] adding affine parallel in lower affine pass to [MLIR] Add lowering for affine.parallel to scf.parallel.Wed, Jul 15, 4:07 AM
bondhugula requested changes to this revision.Wed, Jul 15, 5:09 AM
bondhugula added inline comments.
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
379

lowerBoundValue should actually be called lowerBound and vice versa. Likewise for upper.

385

If the expandAffineMap gives you no value there, you would just be passing empty lowerBound and upperBound. And this could crash.

This revision now requires changes to proceed.Wed, Jul 15, 5:09 AM
yash updated this revision to Diff 278158.Wed, Jul 15, 5:36 AM
yash marked 2 inline comments as done.

Renamed variable and put asserts.

bondhugula accepted this revision.Wed, Jul 15, 10:39 AM

Ping @ftynse - do you have feedback here?

mlir/test/Conversion/AffineToStandard/lower-affine.mlir
640

Can you use a better / more descriptive name than dot1?

This revision is now accepted and ready to land.Wed, Jul 15, 10:39 AM
ftynse accepted this revision.Thu, Jul 16, 3:08 AM

Please address the comments before submitting.

@bondhugula, sorry, I thought this was relanding of a rolled back change

mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
377

Nit: upperBoundValue sounds like it's a _single_ value whereas is a vector of values. This got me confused down the line where the loop is created from upperBoundValue, lowerBoundValue (singular) and steps (plural).

382

Please call steps.reserve before calling push_back in a loop when you know the number of elements to be inserted.

383–384

Nit: would it be possible to construct a ConstantOp and pass it an attribute without extracting its value? Something like rewriter.create<ConstantOp>(loc, step)? Loop bounds are already of index type.

mlir/test/Conversion/AffineToStandard/lower-affine.mlir
640

+1. Also for @dot, I assumed it was a dot product.

yash updated this revision to Diff 278462.Thu, Jul 16, 7:00 AM
yash marked 5 inline comments as done.

Renamed from dot to affine_parallel.
Renamed from lowerBoundValue to lowerBoundTuple to make it plural.
Used steps.reserve.
Not able to use rewriter.create<ConstantOp>(loc, step).

yash updated this revision to Diff 278463.Thu, Jul 16, 7:01 AM

Comment.

ftynse added inline comments.Thu, Jul 16, 7:04 AM
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
380

Actually, do you expect it to never fail (it will fail on quasi-affine maps, for example)? Otherwise, you can just return failure() from the pattern.

Harbormaster completed remote builds in B64511: Diff 278462.
bondhugula added inline comments.Thu, Jul 16, 8:46 AM
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
380

Did you mean quasi-affine or semi-affine? expandAffineMap returns an Optional but doesn't document when it is expected to have a value nor is it obvious from either the declaration or even immediately from the implementation! Goes to show how important documentation is! Do you know who the author of this is - could we have the doc comment updated? Return values like these must be documented without fail.

bondhugula added inline comments.Thu, Jul 16, 8:48 AM
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
380

In theory, we could expand any affine map, but I'm guessing semi-affine maps (where the RHS of the floordiv/ceildiv/mod is a symbol) are perhaps unsupported by expandAffineMap and so this would fail in those cases. So, for completeness sake, we should return failure for now and add a comment that in theory, it's always guaranteed to succeed if expandAffineMap is extended.

yash updated this revision to Diff 278508.Thu, Jul 16, 9:02 AM

Return failure if expandAffineMap fails.

yash updated this revision to Diff 278510.Thu, Jul 16, 9:07 AM
yash marked 4 inline comments as done.

Minor update.

bondhugula accepted this revision.Fri, Jul 17, 11:13 PM
bondhugula added inline comments.Fri, Jul 17, 11:16 PM
mlir/test/Conversion/AffineToStandard/lower-affine.mlir
675

Nit: You can drop the %{{.*}} as well.

yash updated this revision to Diff 278973.Sat, Jul 18, 12:10 AM
yash marked an inline comment as done.

Minor change.

This revision was automatically updated to reflect the committed changes.