Page MenuHomePhabricator

[MLIR] Fix lowering of affine operations with return values.
ClosedPublic

Authored by pr4tgpt on Dec 10 2020, 11:57 PM.

Details

Summary

This commit addresses the issue of lowering affine.for and affine.parallel having return values. Relevant test cases are also added.
Signed-off-by: Prateek Gupta <prateek@polymagelabs.com>

Diff Detail

Event Timeline

pr4tgpt created this revision.Dec 10 2020, 11:57 PM
pr4tgpt requested review of this revision.Dec 10 2020, 11:57 PM
pr4tgpt retitled this revision from [MLIR] Fix lowering of affine operations with return values. This commit addresses the issue of lowering affine.for and affine.parallel having return values. Relevant test cases are also added. to [MLIR] Fix lowering of affine operations with return values..Dec 10 2020, 11:58 PM
pr4tgpt edited the summary of this revision. (Show Details)
bondhugula requested changes to this revision.Dec 11 2020, 12:11 AM
bondhugula added inline comments.
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
369

Why is its argument optional? This will just crash in the switch check if it doesn't have a value!

388

Likewise.

This revision now requires changes to proceed.Dec 11 2020, 12:11 AM
bondhugula added inline comments.Dec 11 2020, 12:14 AM
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
400

Nit: terminate comment with a full stop.

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

No need of the %{{.*}} = part unless you are capturing and matching it.

Thanks for working on this! Just some minor comments from my side, some of them unrelated to the patch, for my understanding :)

mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
368

value of identity value -> identity value?
a Atomic -> an Atomic?

384

Wouldn't this return be unreachable?

467

nit: please, compute the upper bound only once: for (unsigned i = 0, end = reductions.size(); i < end...

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

Unrelated question: is affine analysis able to reason about reductions in this form? I remember a discussion about using mem2reg <--> reg2mem. Has there been any progress on that from?

51

Sorry, a bit unrelated but, while at this... Could we modernize this code and add the special string // ----- for separation and enable -split-input-file?

pr4tgpt updated this revision to Diff 311486.Dec 13 2020, 10:16 PM

First update referring to the comments. Corrected the Optional arguments issue. Also fixed minor issues related to code comments and other optimizations.

pr4tgpt marked 7 inline comments as done.Dec 13 2020, 10:19 PM
pr4tgpt added inline comments.
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
384

Changed the Optional arguments.

pr4tgpt marked an inline comment as done.Dec 13 2020, 10:20 PM
bondhugula requested changes to this revision.Dec 19 2020, 12:47 AM

Mostly minor comments.

mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
387–388

Nit: This comment could be made more accurate.

Return the value obtained by applying the reduction operation kind associated with a binary AtomicRMWKind op` to lhs and rhs`
?

457

Nit: Operation -> operation

462

Use /* ... =*/nullptr for the last argument.

464

Rephrase -> "Copy the body of the affine.parallel op"?

472

You don't need mlir::.

481

Nit: resultReduction -> reductionResult ?

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

UNRELATED: Not really. The affine passes haven't been reviewed to handle reduction values where appropriate.

51

This I suspect may require some non-trivial updates to the test cases. Let's do this in an NFC patch to not confuse this history.

706

These test cases don't have return values. I assume you just added them for additional safety so that the changes don't break these cases?

This revision now requires changes to proceed.Dec 19 2020, 12:47 AM
pr4tgpt updated this revision to Diff 313062.Dec 21 2020, 3:06 AM

Updated few comments and minor changes as suggested.

pr4tgpt marked 7 inline comments as done.Dec 21 2020, 3:08 AM
pr4tgpt added inline comments.
mlir/test/Conversion/AffineToStandard/lower-affine.mlir
706

Yes!

pr4tgpt updated this revision to Diff 313064.Dec 21 2020, 3:15 AM
pr4tgpt marked an inline comment as done.

Updated minor comments.

bondhugula accepted this revision.Dec 22 2020, 3:11 AM
bondhugula added inline comments.
mlir/lib/Conversion/AffineToStandard/AffineToStandard.cpp
387

Triple /// comments please.

448

Nit: having -> with

This revision is now accepted and ready to land.Dec 22 2020, 3:11 AM
pr4tgpt updated this revision to Diff 313279.Dec 22 2020, 3:20 AM

Update minor comments.

pr4tgpt marked 2 inline comments as done.Dec 22 2020, 3:21 AM
bondhugula accepted this revision.Dec 22 2020, 3:36 AM
This revision was landed with ongoing or failed builds.Dec 22 2020, 8:16 AM
This revision was automatically updated to reflect the committed changes.