Page MenuHomePhabricator

[mlir] Normalize the results of normalizable operations
AcceptedPublic

Authored by imaihal on Oct 2 2020, 12:35 AM.

Details

Summary

Memrefs with affine_map in the results of normalizable operation were
not normalized by --normalize-memrefs option. This patch normalizes
them.

Diff Detail

Event Timeline

imaihal created this revision.Oct 2 2020, 12:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 12:35 AM
imaihal requested review of this revision.Oct 2 2020, 12:35 AM
imaihal updated this revision to Diff 295752.Oct 2 2020, 1:19 AM

Fix to remove warning message (newOp -> *newOp)

imaihal added subscribers: avarmapml, bondhugula.EditedOct 2 2020, 2:23 PM

@bondhugula @avarmapml I found the results(returend memref) of normalizable operation were not normalized by --normalize-memrefs.
In test case, %1 was not normalized. This patch fixes it.
%1 = "test.op_norm_ret"(%arg0, %arg1) : (memref<1x32768xf32>, memref<1x16x14x14xf32, #map1>) -> (memref<1x16x14x14xf32, #map1>)

Could you review this patch?

Hi @imaihal, thank you for this patch.
The function normalizeFuncOpMemRefs has become quite long and I believe it'd be better to split the logic into modular sub-functions.
AllocOps, function arguments, result types, and external functions are being addressed within and maybe we can have separate functions for each of them.
Not sure if this change should go into this patch, maybe @bondhugula can say better.

mlir/lib/Transforms/NormalizeMemRefs.cpp
397

Comment should be terminated with a full stop.
Also, I guess the comment needs to be rephrased to Add types to resultTypes``?

bondhugula requested changes to this revision.Oct 3 2020, 10:28 AM
bondhugula added inline comments.
mlir/lib/Transforms/NormalizeMemRefs.cpp
391–455

This is a really long lambda. Please outline.

This revision now requires changes to proceed.Oct 3 2020, 10:28 AM

Thanks for adding this support. Some minor comments. I'll be able to review post the refactoring.

mlir/lib/Transforms/NormalizeMemRefs.cpp
392

You don't need dyn_cast here - !isa<CallOp>(op) will do.

mlir/test/Transforms/normalize-memrefs-ops.mlir
95

Nit: consider using map_tile.

mlir/test/lib/Dialect/Test/TestOps.td
662

-> ... of an op that has normalizable memref results.

bondhugula added inline comments.Oct 3 2020, 10:34 AM
mlir/lib/Transforms/NormalizeMemRefs.cpp
392

Nit: prefer using > 0.

433

If you do a dyn_cast, you have to check for null. If you know these will be memrefs, please use cast.

imaihal updated this revision to Diff 296100.Oct 5 2020, 12:13 AM

Reflected comments and rebased.

imaihal updated this revision to Diff 296198.Oct 5 2020, 9:09 AM

Updated test case to include two operation results.

imaihal updated this revision to Diff 296336.Oct 5 2020, 5:42 PM
imaihal marked 6 inline comments as done.

Rebased and fixed a comment.

imaihal marked an inline comment as done.Oct 5 2020, 5:46 PM

@bondhugula @avarmapml Thanks for your comments! I updated the code to reflect your comments. I moved part of code in the lambda to Util.cpp. Could you review again?

mlir/lib/Transforms/NormalizeMemRefs.cpp
391–455

I moved part of the code to newly created function in Util.cpp. So, I think the lambda is not so long now. Also, I wrote comments about the outline at the top.

397

I wrote comments more in newly created function in Util.cpp.

bondhugula requested changes to this revision.Oct 21 2020, 4:20 PM
bondhugula added inline comments.
mlir/include/mlir/Transforms/Utils.h
149–150 ↗(On Diff #296336)

This has just one user and functionality specific to normalize-memrefs. Why is it exposed?

This revision now requires changes to proceed.Oct 21 2020, 4:20 PM
imaihal updated this revision to Diff 301299.Oct 28 2020, 8:35 AM

Reflected the comment.

imaihal marked an inline comment as done.Oct 28 2020, 8:38 AM
imaihal added inline comments.
mlir/include/mlir/Transforms/Utils.h
149–150 ↗(On Diff #296336)

I moved the function to NormalizedMemrefs

bondhugula accepted this revision.Sat, Nov 7, 9:46 PM

Looks good. Please address the remaining minor comments.

mlir/lib/Transforms/NormalizeMemRefs.cpp
475–479

Triple doc comments please.

mlir/test/Transforms/normalize-memrefs-ops.mlir
99

%[[v0:.*]] = should be sufficient.

This revision is now accepted and ready to land.Sat, Nov 7, 9:46 PM
imaihal updated this revision to Diff 304399.Tue, Nov 10, 10:00 PM
imaihal marked an inline comment as done.

Rebased and reflected comments.

imaihal marked 2 inline comments as done.Tue, Nov 10, 10:04 PM
imaihal added inline comments.
mlir/test/Transforms/normalize-memrefs-ops.mlir
99

I also fixed other variables from [a-z0-9]* to .* in this file.

imaihal marked an inline comment as done.Tue, Nov 10, 10:22 PM

Looks good. Please address the remaining minor comments.

@bondhugula Thanks for your review. I reflected your comments. Could you commit this patch if you don't have any other comments? I don't have commit access.