Page MenuHomePhabricator

[mlir] [VectorOps] Canonicalization of 1-D memory operations
ClosedPublic

Authored by aartbik on Aug 11 2020, 12:18 PM.

Details

Summary

Masked loading/storing in various forms can be optimized
into simpler memory operations when the mask is all true
or all false. Note that the backend does similar optimizations
but doing this early may expose more opportunities for further
optimizations. This further prepares progressively lowering
transfer read and write into 1-D memory operations.

Diff Detail

Event Timeline

aartbik created this revision.Aug 11 2020, 12:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 12:18 PM
aartbik requested review of this revision.Aug 11 2020, 12:18 PM
bondhugula requested changes to this revision.Aug 11 2020, 1:15 PM
bondhugula added a subscriber: bondhugula.

Documentation and comments are missing throughout.

mlir/lib/Dialect/Vector/VectorOps.cpp
41

Missing doc comment.

46

Avoid auto here.

60

Avoid auto.

63

Comment for this block.

72

Doc comment doesn't describe the method. Output ref parameters not documented. Triple doc comment.

This revision now requires changes to proceed.Aug 11 2020, 1:15 PM
aartbik added inline comments.Aug 11 2020, 1:20 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
46

What is your reasoning on when auto is okay and when not?

bondhugula added inline comments.Aug 11 2020, 1:27 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
46

The reasoning is not mine, but what's being followed in other revisions I've seen and repeated by @mehdi_amini and @rriddle as well multiple times is to avoid using auto where it does NOT improve readability. The usage here is hurting readability however obvious it might appear to you now.

@rriddle @mehdi_amini - shouldn't we be documenting this guideline in the style guidelines on mlir.llvm.org?

aartbik updated this revision to Diff 284876.Aug 11 2020, 1:40 PM
aartbik marked 4 inline comments as done.

addressed comments

aartbik marked 2 inline comments as done.Aug 11 2020, 1:43 PM
aartbik added inline comments.
mlir/lib/Dialect/Vector/VectorOps.cpp
46

Just to make sure, I was not disagreeing with your feedback (I usually follow the advice of my Google mentor from long time back that the reviewer is almost always right :-), I was genuinely interested in some form of guideline on this.

aartbik marked an inline comment as done.Aug 11 2020, 1:47 PM

Oh, and I would be very interested in seeing your work on memref casting being upstreamed, just to unify some of the vector specific ops we have been using so far!

Oh, and I would be very interested in seeing your work on memref casting being upstreamed, just to unify some of the vector specific ops we have been using so far!

Sure - I'm submitting that revision for review today.

bondhugula added inline comments.Aug 12 2020, 9:12 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
46

No worries. :) We should definitely have a guideline on the "coding style" page since this has come up often. I'll bring this up on discord.

bondhugula resigned from this revision.Aug 12 2020, 9:13 PM

I'll defer to the original reviewers on the list for a real review!

Nicolas is not going to look at this soon, so can I ask any of the other reviewers to please have a look?

rriddle added inline comments.Aug 13 2020, 1:10 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
46

Please put functions at the top level, marked with static, and not within a namespace.

re: auto (and the comment above), these are in the LLVM coding standards:
https://llvm.org/docs/CodingStandards.html
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable

53

Are these all i1? If so, use denseElts.getValues<bool>().

ThomasRaoux accepted this revision.Aug 13 2020, 1:36 PM

Looks good to me

mlir/lib/Dialect/Vector/VectorOps.cpp
84

nit: the function name suggests that it doesn't have side-effects even though it creates the cast if possible. Maybe there is a better name?

This revision is now accepted and ready to land.Aug 13 2020, 1:36 PM
aartbik updated this revision to Diff 285515.Aug 13 2020, 4:01 PM
aartbik marked 2 inline comments as done.

addressed comments

Thanks River and Thomas. I removed the namespace, used static, and renamed to "castedToMemRef" to make the side effect a bit more clear.....

mlir/lib/Dialect/Vector/VectorOps.cpp
46

Marked with static. Removed namespace.

84

ok, rephrased slightly to make that more clear.