Page MenuHomePhabricator

[mlir][normalize-memrefs] Non-normalizable operations with identity map layouts do not block normalization of the entire function
Needs ReviewPublic

Authored by tungld on May 17 2022, 11:36 PM.

Details

Summary

The current approach is convervative in which whenever there is a non-normalizable operations in a function will the function be labelled as non-normalizable. It means it requires that all operations must have MemRefsNormalizable trait.

This patch relaxes the requirement that if the memref map layouts of a non-normalizable operation are identity, this operation does not block the normalization of the other operations in the same function.

Diff Detail

Event Timeline

tungld created this revision.May 17 2022, 11:36 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 11:36 PM
tungld requested review of this revision.May 17 2022, 11:36 PM
bondhugula added inline comments.May 18 2022, 9:55 AM
mlir/lib/Dialect/MemRef/Transforms/NormalizeMemRefs.cpp
163–164

This isn't the right to check for an identity layout. Instead, use isIdentity() on the layout itself.

178–179

Likewise: here and below.

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

Is this the right test case? In this case, all users of %arg0 are already normalizable and %arg0 doesn't have an identity layout map. Am I missing something?

tungld updated this revision to Diff 430558.May 18 2022, 7:21 PM
  • Use isIdentity() on the layout itself
tungld marked 2 inline comments as done.May 18 2022, 7:28 PM
tungld added inline comments.
mlir/lib/Dialect/MemRef/Transforms/NormalizeMemRefs.cpp
163–164

Done. Thanks!

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

Yes, it is. Without this patch, %arg0 will not be normalized and the whole function stays unchanged. That's because the pass detects that the op_nonnorm op does not have MemRefsNormalizable trait no matter the op's arguments' map layouts are identity.

tungld marked an inline comment as done.May 19 2022, 12:47 AM

@bondhugula Is the current change ok? or should I make any other change? Thanks!

@bondhugula @avarmapml @imaihal could you please review this patch again? Thanks!