Page MenuHomePhabricator

[mlir] Set alignment in AllocOp of normalizeMemref()
ClosedPublic

Authored by imaihal on Jul 12 2020, 11:57 PM.

Details

Summary

AllocOp is updated in normalizeMemref(AllocOp allocOp), but, when the
AllocOp has alignment attribute, it was ignored and updated AllocOp
does not have alignment attribute. This patch fixes it.

Diff Detail

Event Timeline

imaihal created this revision.Jul 12 2020, 11:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2020, 11:57 PM
imaihal edited the summary of this revision. (Show Details)Jul 13 2020, 5:29 PM
imaihal edited the summary of this revision. (Show Details)
mehdi_amini added inline comments.Jul 13 2020, 9:56 PM
mlir/lib/Transforms/Utils/Utils.cpp
454

Can you remove braces here? The block is trivial

455

Can't there be a value() argument to the alloc in this case as well?

Actually do we need a if or can we always write the first form and the alignmentAttr() will just be null?

bondhugula requested changes to this revision.Jul 14 2020, 12:23 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Transforms/Utils/Utils.cpp
449

Nit: Please terminate comment with a period. They also have to be complete sentences.

450–455

The following will instead work:

auto newAlloc = b.create<AllocOp>(allocOp.getLoc(), newMemRefType, llvm::None,
                                    allocOp.alignmentAttr());

You don't need value() - it only works for static memrefs. If one extends this for dynamic memrefs, one simply can't pass allocOp.value() here.

mlir/test/Transforms/memref-normalize.mlir
150

No need of %{{.*}} =

This revision now requires changes to proceed.Jul 14 2020, 12:23 AM
imaihal updated this revision to Diff 277696.Jul 14 2020, 1:03 AM

Remove redundant code

imaihal marked 7 inline comments as done.Jul 14 2020, 1:07 AM

Thanks for your comments! I updated.

mlir/lib/Transforms/Utils/Utils.cpp
449

Code becomes simple. so I removed the comment.

450–455

Thanks for your comments! I didn't know llvm:None works. I used value() because this function currently only support static memrefs, but I changed it.

imaihal marked 2 inline comments as done.Jul 14 2020, 1:07 AM
bondhugula accepted this revision.Jul 14 2020, 7:06 AM

Thanks for fixing this.

This revision is now accepted and ready to land.Jul 14 2020, 7:06 AM
bondhugula added inline comments.Jul 14 2020, 7:07 AM
mlir/test/Transforms/memref-normalize.mlir
150

Can you use CHECK-NEXT here?

imaihal marked an inline comment as done.Jul 14 2020, 5:27 PM
imaihal added inline comments.
mlir/test/Transforms/memref-normalize.mlir
150

Do you mean I need to just replace CHECK with CHECK-NEXT ? Yes, I can do, but why CHECK-NEXT is better?

imaihal updated this revision to Diff 278035.Jul 14 2020, 6:05 PM

Change CHECK to CHECK-NEXT

imaihal updated this revision to Diff 278039.EditedJul 14 2020, 6:31 PM

Rebase

imaihal updated this revision to Diff 278051.Jul 14 2020, 7:23 PM

Rebase again

@bondhugula Thanks for your review! I reflected your comments.
I think I don't have right to land this patch. Could you land this if you don't have additional comments?

@bondhugula Thanks for your review! I reflected your comments.
I think I don't have right to land this patch. Could you land this if you don't have additional comments?

Can you check "Done" on the pending comment?

bondhugula accepted this revision.Sun, Jul 19, 10:59 PM
imaihal marked an inline comment as done.Sun, Jul 19, 11:25 PM
bondhugula marked an inline comment as done.Mon, Jul 20, 3:11 AM
bondhugula added inline comments.
mlir/test/Transforms/memref-normalize.mlir
150

CHECK-NEXT would be more stringent - not allowing anything else in between, which is the case here since you don't expect anything in between.

imaihal marked an inline comment as done.Tue, Jul 21, 11:15 PM
imaihal added inline comments.
mlir/test/Transforms/memref-normalize.mlir
150

I see. Thanks!

Su

@bondhugula Thanks for your review! I reflected your comments.
I think I don't have right to land this patch. Could you land this if you don't have additional comments?

Sure, I'll commit it.

This revision was automatically updated to reflect the committed changes.