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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
mlir/lib/Dialect/Vector/VectorOps.cpp | ||
---|---|---|
46 | What is your reasoning on when auto is okay and when not? |
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? |
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. |
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!
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. |
Nicolas is not going to look at this soon, so can I ask any of the other reviewers to please have a look?
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: | |
53 | Are these all i1? If so, use denseElts.getValues<bool>(). |
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? |
Missing doc comment.