This is an archive of the discontinued LLVM Phabricator instance.

MemRef Normalization for Dialects
ClosedPublic

Authored by AlexEichenberger on Aug 19 2020, 11:33 AM.

Details

Summary

When dealing with dialects that will results in function calls to external libraries, it is important to be able to handle maps as some dialects may require mapped data. Before this patch, the detection of whether normalization can apply or not, operations are compared to an explicit list of operations (alloc, dealloc, return) or to the presence of specific operation interfaces (AffineReadOpInterface, AffineWriteOpInterface, AffineDMAStartOp, or AffineDMAWaitOp).

This patch add a trait, MemRefsNormalizable to determine if an operation can have its memrefs normalized.

This trait can be used in turn by dialects to assert that such operations are compatible with normalization of memrefs with nontrivial memory layout specification. An example is given in the literal tests.

Diff Detail

Event Timeline

AlexEichenberger requested review of this revision.Aug 19 2020, 11:33 AM

remove spurious commented code

bondhugula added a comment.EditedAug 19 2020, 2:07 PM

Thanks for contributing this. You are missing the documentation and the definition of it in doc/Traits.md. A reviewer would typically start form there :-) - to make sure the semantics are understood. Also, the way you've defined it in the commit summary, it doesn't immediately look like DeallocOp and ReturnOp or anything return like would fall into that class and those would still have to be hardcoded / treated specially the way it is. We probably need to iterate/refine on the formal definition of this trait.

Commit summary:
may requires -> may require

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
661

Please keep the trait names in sorted order.

1392

Likewise.

mlir/include/mlir/IR/OpBase.td
1701

All comments are to be terminated with a period.

mlir/include/mlir/IR/OpDefinition.h
1215

Typo here. a the

1215

Please spell out the entire definition of this trait - here and in doc/Traits.md.

bondhugula requested changes to this revision.Aug 19 2020, 2:07 PM
This revision now requires changes to proceed.Aug 19 2020, 2:07 PM
AlexEichenberger edited the summary of this revision. (Show Details)

Added requested doc, edited the summary to be more specific with respect to the contribution of this patch, and removed a prior TODO with respect to recognizing operations that can be normalized by systematically using the new trait.

Added revisions suggested for comments.

added proper order for traits

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
661

reordered systematically.

1392

done everywhere. Some had interface and traits, still ordered alphabetically. Let me know if this is not how it is done.

mlir/include/mlir/IR/OpDefinition.h
1215

Added the refs in doc/Traits and fixed the comment.

Looks good to me. Some superficial comments - first round.

mlir/docs/Traits.md
279

Naturalizable -> Normalizable

283

Nit: Spell fix: accommodate

285

Nit: proceed -> be performed

287

-> with non-identity layout ..?

mlir/include/mlir/IR/OpBase.td
1701

Nit: in -> in the

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

Nit: drop extra line.

  1. Stretch question: do you see a scenario where some operand memrefs are normalizable but not others?

Since this 'MemRefsNormalizable' trait is more of "can/may be normalized" and is a first pass check because there are other things that could prevent it. For eg., CallOp is marked with this trait, but if a memref argument for the callee isn't normalizable due to a user op in that callee where it's not normalizable, that call op won't be normalized and transitively other ops won't be as well.

In the future or as a TODO, I think the right way to model this is using SideEffects, in particular, MemoryEffects (I briefly mentioned this on thread). This allows you to specify an effect at operand granularity. The design will require more thought though.

bondhugula requested changes to this revision.Aug 22 2020, 8:58 AM
bondhugula added inline comments.
mlir/test/mlir-tblgen/op-memrefs-norm.mlir
2

The test case is in the wrong directory!

5

Please append these test cases to the existing ones for normalize-memrefs, or at least in the same directory.

This revision now requires changes to proceed.Aug 22 2020, 8:58 AM
bondhugula added inline comments.Aug 22 2020, 9:01 AM
mlir/test/mlir-tblgen/op-memrefs-norm.mlir
3

Please have a line or two here on what these test cases are testing.

11–15

Please don't use numbered SSA values in test cases. They can't be maintained - either %{{.*}} if you don't care about capturing and comparing, or capture it and use it. On the LHS, if you aren't capturing, you can even drop the part to the left of = ....

24–26

Likewise.

29

It would be good to have a line saying what this is testing for readability in the future.

36–38

No numbered SSA ids in check lines please.

avarmapml accepted this revision.Aug 24 2020, 12:29 AM

Looks good to me.

As suggested by @bondhugula the transitive dependency of normalizable memrefs needs to be taken into account because currently that's how the normalize-memrefs pass works.
Can be added as TODO.

Implemented all of the requested changes. Introduced at TODO for partially normalizable operations.

mlir/docs/Traits.md
287

Implemented all of the requested text changes.

mlir/include/mlir/IR/OpBase.td
1701

changed the comment.

mlir/include/mlir/IR/OpDefinition.h
1215

Added the text of doc/Traits.md here as well, if that is what you asked.

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

Removed the extra line.

  1. Stretch question: do you see a scenario where some operand memrefs are normalizable but not others?

Since this 'MemRefsNormalizable' trait is more of "can/may be normalized" and is a first pass check because there are other things that could prevent it. For eg., CallOp is marked with this trait, but if a memref argument for the callee isn't normalizable due to a user op in that callee where it's not normalizable, that call op won't be normalized and transitively other ops won't be as well.

In the future or as a TODO, I think the right way to model this is using SideEffects, in particular, MemoryEffects (I briefly mentioned this on thread). This allows you to specify an effect at operand granularity. The design will require more thought though.

Good suggestion; I am leaving it as a TODO as this patch is already extending prior functionality to other dialects. Your approach should guide us when we will want to handle such operations.

rriddle added inline comments.Aug 25 2020, 1:47 AM
mlir/docs/Traits.md
279

Keep these sorted.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2129

Please format this such that the traits are aligned.

mlir/lib/Transforms/Utils/Utils.cpp
282

nit: Looks like an extra space here.

mlir/test/Transforms/normalize-memrefs-ops.mlir
1 ↗(On Diff #287475)

Why are there multiple options here? You should generally only have one to keep the test focused, and not rely on the compounded behavior.

Update to reflect latest comments.

mlir/docs/Traits.md
279

Make sense, done.

mlir/include/mlir/Dialect/StandardOps/IR/Ops.td
2129

aligned them.

mlir/lib/Transforms/Utils/Utils.cpp
282

Good catch, removed.

mlir/test/Transforms/normalize-memrefs-ops.mlir
1 ↗(On Diff #287475)

I mistakenly believed that I needed the other option because the test uses a test op. Indeed it is not needed. Removed.

bondhugula accepted this revision.Aug 25 2020, 7:56 AM

Some more minor comments - mostly on documentation and comments. Can you please fix?

mlir/docs/Traits.md
257–258

Not sure you should be using MemRefs in plural here. Is this for cross-referencing? I think you'll have to use a different syntax for that with a #.

259

access pattern -> access

-> .. is applied by rewriting accesses ...

260

this memory references -> those memref references or that memory reference.

mlir/include/mlir/IR/OpBase.td
1701

-> with non-identity maps ? Memrefs always have maps.

mlir/include/mlir/IR/OpDefinition.h
1220–1221

To be fixed in line with the trait doc.

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

Comment here please.

This revision is now accepted and ready to land.Aug 25 2020, 7:56 AM

performed all requested changes but one. Left a clarification request for the suggestion that I am not sure to understand properly.

mlir/docs/Traits.md
257–258

@bondhugula Are you asking to rename the trait to MemRefNormalizable where the "s" is removed ? I just want to make sure I understood your suggestion.

I am fine either way. In general, I don't like "s" in the middle. If it was up to my choice, I would have preferred no "s" to MemRef either. The trait property is on the operation so some of them have multiple MemRefs.

Just let me know how you want to go and I will make necessary changes.

260

performed the changes

mlir/include/mlir/IR/OpBase.td
1701

good catch, changed.

mlir/include/mlir/IR/OpDefinition.h
1220–1221

copied it over

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

Added as requested.

bondhugula added inline comments.Aug 25 2020, 9:01 AM
mlir/docs/Traits.md
257–258

I wasn't actually thinking about the trait renaming - in fact, I didn't think about that part. I was referring to MemRefs you were using further below which don't parse well in their sentences

an original MemRefs

an equivalent MemRefs

Are you referring to plural or singular? These are broken and have to be fixed.

Completed all current requests for changes.

mlir/docs/Traits.md
257–258

make sense; done.

bondhugula accepted this revision.Aug 25 2020, 8:33 PM

Thanks!

@AlexEichenberger Let me know if you are able to commit or if you'd like me to commit this for you.

@bondhugula I don't believe I have commit privilege yet. Please commit for me, if you don't mind.

Thanks to all for your helpful comments, especially for guiding me toward the right approach. Happy to add the TODOs when the need arises.

@bondhugula I don't believe I have commit privilege yet. Please commit for me, if you don't mind.

Sure. To confirm, should I be using these credentials on the commit: "Alexandre E. Eichenberger <alexe@us.ibm.com>"? In previous ones, I see another address as well.

This revision was automatically updated to reflect the committed changes.