Page MenuHomePhabricator

[MLIR] Support for return values in Affine.For yield
ClosedPublic

Authored by avarmapml on Thu, Sep 10, 12:22 AM.

Details

Summary
  • Added support for return values in affine.for yield along the same lines as scf.for and affine.parallel.

Signed-off-by: Abhishek Varma <abhishek.varma@polymagelabs.com>

Diff Detail

Event Timeline

avarmapml created this revision.Thu, Sep 10, 12:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Sep 10, 12:22 AM
avarmapml requested review of this revision.Thu, Sep 10, 12:22 AM
avarmapml edited the summary of this revision. (Show Details)Thu, Sep 10, 12:28 AM
bondhugula added inline comments.
mlir/include/mlir/Dialect/Affine/IR/AffineOps.td
303

Format fix please.

306

This should return an unsigned.

bondhugula requested changes to this revision.Thu, Sep 10, 9:02 AM
bondhugula added inline comments.
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1248–1252

Nit: > 0

1413

operandType

1421

Nit: terminate with full stop.

1483–1484

I think both these methods return unsigned.

mlir/test/Dialect/Affine/invalid.mlir
386

Consider 'defined values' -> 'results'
mismatch in number -> mismatch between the number
?

This revision now requires changes to proceed.Thu, Sep 10, 9:02 AM

LGTM! Just some nit comments! Thanks for contributing this feature!

This commit does not provide support for reduction var.

What do you mean exactly? The example in the doc is a reduction. Do you mean that this patch doesn't include the analysis to deal with reductions described using iter_args?

mlir/include/mlir/Dialect/Affine/IR/AffineOps.td
231

some doc here about what this is for would be great

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1179

Same: doc here about bodyBuilder would be useful.

1191

nit: v -> val/value,...

1212

same

mlir/test/Dialect/Affine/invalid.mlir
384

nit: affine_for -> something more descriptive? affine_for_iter_args_mismatch?

mlir/test/Dialect/Affine/ops.mlir
211

nit: affine.if indentation++

221

nit: affine_for_multiple_yield?

avarmapml marked 14 inline comments as done.

Tackled initial review comments.

Bug fix for build function.

Harbormaster completed remote builds in B71329: Diff 291139.
bondhugula requested changes to this revision.Mon, Sep 14, 4:10 AM

I think we definitely need a test for the builder API here to make sure it's working as intended. Please see the other builder API tests for reference in test/EDSC/builder-api-test.cpp.

This revision now requires changes to proceed.Mon, Sep 14, 4:10 AM

Added test case for build method and bug fix

  • getUpperBoundOperands() was not picking up operands correctly. This update fixes that.
  • Added a minor test case to check build method for affine.for along with iter_args.
  • Updated function affineLoopBuilder definition and updated the same in builder-api-test.cpp.
avarmapml updated this revision to Diff 291719.Mon, Sep 14, 3:59 PM

Updated getter/setter functions for lower and upper bound

bondhugula added inline comments.Mon, Sep 14, 11:25 PM
mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1627–1629

We should update the test cases to make sure these are covered - without this change, print would have been printing the wrong thing. You could add a test case that has upper bound operands as well as iter arguments.

bondhugula requested changes to this revision.Mon, Sep 14, 11:28 PM

Thanks for fixing the issues.

mlir/test/EDSC/builder-api-test.cpp
180

builder_for_iter_arg -> builder_affine_for_iter_args?

194

Please also match the body with the yield.

737

/*iterArgs=*/llvm::None

This revision now requires changes to proceed.Mon, Sep 14, 11:28 PM
bondhugula added inline comments.Mon, Sep 14, 11:33 PM
mlir/include/mlir/Dialect/Affine/IR/AffineOps.td
179–180

3 loop control values ... <-- this would be inaccurate (copy/paste error).

184

Nit: a -> an

302–303

This would be inaccurate - since the init operands for the iter args shouldn't be counted here. Please check if this accessor is needed and what the intended use is.

mlir/lib/Dialect/Affine/IR/AffineOps.cpp
1486

You can just do getNumOperands() with getOperation(). Also, compactly:

return getNumOperands() - lbMap.getNumInputs() - ubMap.getNumInputs();
avarmapml updated this revision to Diff 291839.Tue, Sep 15, 2:48 AM
avarmapml marked 8 inline comments as done.

Handled review comments and updated test case

avarmapml edited the summary of this revision. (Show Details)Tue, Sep 15, 3:16 AM
bondhugula requested changes to this revision.Tue, Sep 15, 9:22 PM
bondhugula added inline comments.
mlir/test/EDSC/builder-api-test.cpp
194

Looks like this hasn't been addressed, but marked done. Did you missing pushing?

This revision now requires changes to proceed.Tue, Sep 15, 9:22 PM

Updated test case to match loop body/yield

avarmapml marked an inline comment as done.Thu, Sep 17, 12:29 AM
bondhugula added inline comments.Thu, Sep 17, 1:46 AM
mlir/test/EDSC/builder-api-test.cpp
195–196

You don't need SmallVector here - instead, ValueRange(.., ..) inline would work.

bondhugula accepted this revision.Thu, Sep 17, 1:46 AM

Looks good, thanks!

This revision is now accepted and ready to land.Thu, Sep 17, 1:46 AM
avarmapml updated this revision to Diff 292427.Thu, Sep 17, 1:54 AM
avarmapml marked an inline comment as done.

Minor code refactor

bondhugula accepted this revision.Thu, Sep 17, 2:05 AM
This revision was automatically updated to reflect the committed changes.