This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Updates around MemRef Normalization
ClosedPublic

Authored by stephenneuendorffer on Sep 29 2020, 11:51 PM.

Details

Summary

The documentation for the NormalizeMemRefs pass and the associated MemRefsNormalizable
traits was confusing and not on the website. This update clarifies the language
around the difference between a MemRef Type, an operation that accesses the value of
MemRef Type, and better documents the limitations of the current implementation.
This patch also includes some basic debugging information for the pass so people
might have a chance of figuring out why it doesn't work on their code.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
stephenneuendorffer requested review of this revision.Sep 29 2020, 11:51 PM
rriddle accepted this revision.Sep 30 2020, 11:23 AM

Nice. Please run clang-format though.

mlir/include/mlir/Transforms/Passes.td
375

nit: Add a newline before here.

422

nit: Drop the newline here.

This revision is now accepted and ready to land.Sep 30 2020, 11:23 AM
bondhugula requested changes to this revision.Oct 1 2020, 1:27 AM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/docs/Traits.md
254–263

Hmm... this isn't entirely accurate. We may still need to tweak/improve the original thing. Ops like DeallocOp, CallOp, and ReturnOp also have this trait but they don't access the memref through an arbitrary affine subscript function. The trait is more about when you can normalize the memref operand on the op safely / meaningfully.

This revision now requires changes to proceed.Oct 1 2020, 1:27 AM

Thanks for submitting this - esp. the detailed pass description. We should definitely improve the documentation since you didn't find it clear enough.

Added suggestion, thanks for attempting to clarify the original text, much appreciated.

mlir/include/mlir/Transforms/Passes.td
327

"must" That is probably too strong, as operations can be handled directly in the pass. I would say what the flag does, namely it will let the pass assume that normalizing that operation is safe.

331

"Currently...." maybe this is too strong a statement as we are adding support, and then we would have to correct this list. I would prefer something weaker. "Initial support includes...."

337

Nit: all passes have to be conservative in general. I would just simply state the current limitation.

Currently the pass is limited to handle functions where all ...

and if you feel like adding this

We may address this limitation if the need arises in the future.

Fine either ways

stephenneuendorffer marked 5 inline comments as done.Oct 1 2020, 11:14 AM

Thanks for the feedback.

mlir/docs/Traits.md
254–263

I reworded this.

A few more minor comments.

mlir/docs/Traits.md
254–262

access -> use? (access could imply dereferencing, which isn't the case with dealloc or call).

258

it's -> its

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

Likewise.

1216–1218

Looks like this hasn't been updated.

mlir/include/mlir/Transforms/Passes.td
317–325

Thanks very much for this description.

stephenneuendorffer marked 5 inline comments as done.Oct 1 2020, 8:02 PM
bondhugula accepted this revision.Oct 1 2020, 9:00 PM
This revision is now accepted and ready to land.Oct 1 2020, 9:00 PM
This revision was automatically updated to reflect the committed changes.