This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Affine] Add affine.parallel op
ClosedPublic

Authored by flaub on Feb 8 2020, 12:49 PM.

Details

Summary

As discussed in https://llvm.discourse.group/t/rfc-add-affine-parallel/350, this is the first in a series of patches to bring in support for the affine.parallel operation.

This first patch adds the IR representation along with custom printer/parser implementations.

Diff Detail

Unit TestsFailed

Event Timeline

flaub created this revision.Feb 8 2020, 12:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2020, 12:49 PM
earhart accepted this revision.Feb 10 2020, 11:58 AM

LGTM

mlir/include/mlir/Dialect/AffineOps/AffineOps.td
288

This sentence seems slightly awkward -- maybe something like,

Calling AffineParallelOp::build will automatically create the required region and block, and insert the required terminator.  Parsing will also create the required region, block, and terminator, even when they are missing from the textual representation.

? (Or maybe it's better to keep as-is, for consistency with AffineForOp...?)

This revision is now accepted and ready to land.Feb 10 2020, 11:58 AM
jbruestle accepted this revision.Feb 10 2020, 12:13 PM

Looks good.

mlir/include/mlir/Dialect/AffineOps/AffineOps.td
298

Why use square brackets rather than parens? In general, tuples are usually parens.

mlir/lib/Dialect/AffineOps/AffineOps.cpp
140–141

I suspect this may mean that certain analysis/transforms may now fail (possibly via an assert?) if AffineParallelOp is used, but I think that probably can be fixed in later commits since nothing currently introduces AffineParallelOp yet.

flaub added inline comments.Feb 10 2020, 2:21 PM
mlir/include/mlir/Dialect/AffineOps/AffineOps.td
298

This stems from the usage of parseAffineMapOfSSAIds, it apparently assumes square bracket delimiters.

@rriddle Would it break things if we allowed parseAffineMapOfSSAIds to take a delimiter so that we could group these ids in parens? I'm worried that this might cause ambiguity in the parser.

Some small comments. With changes syntax this looks great.

mlir/include/mlir/Dialect/AffineOps/AffineOps.td
298

I would prefer () here, as well. I do not see an issue with changing the bracket style. Affine expressions should have matching opening/closing brackets, so the parser should not get confused by an extra closing bracket.

307

You could use SameVariadicOperandSize trait and then split the mapOperands into upper and lower bound here instead of implementing explicitly.

mlir/lib/Dialect/AffineOps/AffineOps.cpp
2290–2299

Maybe if (llvm::any_of(steps(), ...)) {

2342

Maybe if (failed(parser... to make it more obvious that the first one is the failure case?

2347–2363

Could this code just parse a list of constant integers directly?

flaub updated this revision to Diff 243804.Feb 11 2020, 4:07 AM
flaub marked 13 inline comments as done.
  • Review feedback
This revision now requires review to proceed.Feb 11 2020, 4:07 AM
flaub marked 2 inline comments as done.Feb 11 2020, 4:09 AM
flaub added inline comments.
mlir/include/mlir/Dialect/AffineOps/AffineOps.td
288

Re-worded, thanks.

298

I extended the Parser to support specifying the delimiter to use with parseAffineMapOfSSAIds, and this works!

307

Great, TIL :)

mlir/lib/Dialect/AffineOps/AffineOps.cpp
2290–2299

In order to print an array wrapped with parens, I needed to collect the elements of the array anyways. But I'm glad I learned about more LLVM functional helpers.

2342

SGTM

2347–2363

The only other Parser method available that I could find suitable was parseAttribute into an ArrayAttr, however this expects square brackets.

Should we extend the Parser to support this or perhaps save it for another (NFC) day?

flaub updated this revision to Diff 243805.Feb 11 2020, 4:16 AM
flaub marked an inline comment as done.
  • Improve error msg
rriddle requested changes to this revision.Feb 11 2020, 9:02 AM
rriddle added inline comments.
mlir/include/mlir/Dialect/AffineOps/AffineOps.td
335

Drop all of the llvm:: and mlir::

mlir/lib/Dialect/AffineOps/AffineOps.cpp
2168

nit: I would restructure this given that all of the lb elements are the same.

SmallVector<AffineExpr, 8> ubExprs;
for (int64_t range : ranges)
  ubExprs.push_back(builder->getAffineConstantExpr(range));

SmallVector<AffineExpr, 8> lbExprs(ubExprs.size(), builder->getAffineConstantExpr(0));
2176

nit: Can you change all of these comments to be complete sentences?

2200

Missing punctuation on each of these. Also please make them complete sentences.

2210

Remove the trivial braces.

2214

This is unnecessary.

2242

Remove trivial braces.

2242

Can you change this to early exit instead?

2254

Remove all of the mlir:: and llvm::.

2285

Remove trivial braces. Also, you can just do something like:

elideSteps &= (step == 1);
2315

nit: This is flowing to the next line, so just split it off and repeat the type.

This revision now requires changes to proceed.Feb 11 2020, 9:02 AM
jbruestle requested changes to this revision.Feb 11 2020, 10:20 AM
jbruestle added inline comments.
mlir/include/mlir/Dialect/AffineOps/AffineOps.td
307

I don't see any reason that both maps would always take the same number of operands. For example, if one wanted do:

affine.parallel (%i, %j) = (0,0) to (sym(N), sym(M)) {
  ...
}

which seems like a common enough use case, then lowerBoundOperands would be empty and upperBoundOperands would have 2 symbols. I think the use of SameVariadicOperandSize should be reverted.

flaub updated this revision to Diff 243954.Feb 11 2020, 12:23 PM
flaub marked 10 inline comments as done.
  • Review updates

Thanks, more changes incoming.

jbruestle accepted this revision.Feb 11 2020, 12:29 PM
flaub marked 3 inline comments as done.Feb 11 2020, 2:54 PM
flaub added inline comments.
mlir/lib/Dialect/AffineOps/AffineOps.cpp
2208

ensureTermniator is unnecessary, or just the redundant comment line?

nicolasvasilache accepted this revision.Feb 11 2020, 5:58 PM

Thanks for pushing on this, LGTM!

@rriddle Are you OK with the latest changes? Thanks :)

rriddle accepted this revision.Feb 12 2020, 10:20 AM

Looks great, thanks!

mlir/include/mlir/Dialect/AffineOps/AffineOps.td
287

nit: Can you move Calling AffineParallelOp::... down to a Note: section?

mlir/lib/Dialect/AffineOps/AffineOps.cpp
2208

The comment, sorry for the confusion.

2214

nit: you could do getOperands().take_front(lowerBoundsMap().getNumInputs())

2218

nit: Similarly here, but with drop_front.

2274

nit: Can you stream the arguments?

2371

nit: Remove the newline here.

This revision is now accepted and ready to land.Feb 12 2020, 10:20 AM
bondhugula accepted this revision.Feb 12 2020, 10:53 AM
bondhugula added inline comments.
mlir/lib/Dialect/AffineOps/AffineOps.cpp
2180

Nit: dims -> dim or numDims

2193

Nit: -> Verify that the dimensionality of the maps matches the number of steps.

2238

out.reserve(rangesValueMap.getNumResults());

2257

Nit: message please - && "fewer/too many steps"

2262

Nit: dims -> numDims / nDims / dim ?

bondhugula added inline comments.Feb 12 2020, 11:10 AM
mlir/lib/Dialect/AffineOps/AffineOps.cpp
2267

If you'd like to have all the canonicalizations surrounding affine operations and extend some of the affine analysis routines to this, you'll need to call verifyDimAndSymbolIdentifiers on the map operands. I don't think any of your current test cases will break, but you'll have to add a couple of invalid test cases where one of your map operands isn't a symbol - for eg. a value loaded right inside an outer loop.

flaub marked 13 inline comments as done.Feb 12 2020, 5:50 PM
flaub added inline comments.
mlir/lib/Dialect/AffineOps/AffineOps.cpp
2267

Thanks! Done.

2274

Looks like no because getArguments() returns a MutableArrayRef<Value> and currently it appears that ValueRange isn't constructible from this.

flaub updated this revision to Diff 244309.Feb 12 2020, 5:51 PM
flaub marked 2 inline comments as done.
  • - Review updates
rriddle added inline comments.Feb 12 2020, 5:53 PM
mlir/lib/Dialect/AffineOps/AffineOps.cpp
2274

It should be possible as of this morning.

flaub marked an inline comment as done.Feb 12 2020, 5:59 PM
flaub added inline comments.
mlir/lib/Dialect/AffineOps/AffineOps.cpp
2274

Right you are :) done.

flaub updated this revision to Diff 244311.Feb 12 2020, 6:00 PM
  • stream
This revision was automatically updated to reflect the committed changes.
Harbormaster completed remote builds in B46370: Diff 244309.