This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Affine][VectorOps] Vectorize uniform values in SuperVectorizer
ClosedPublic

Authored by dcaballe on Aug 27 2020, 7:38 PM.

Details

Summary

This patch adds basic support for vectorization of uniform values to SuperVectorizer.
For now, only invariant values to the target vector loops are considered uniform. This
enables the vectorization of loops that use function arguments and external definitions
to the vector loops. We could extend uniform support in the future if we implement some
kind of divergence analysis algorithm.

Diff Detail

Event Timeline

dcaballe created this revision.Aug 27 2020, 7:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2020, 7:38 PM
dcaballe requested review of this revision.Aug 27 2020, 7:38 PM
rriddle added inline comments.Aug 28 2020, 1:01 PM
mlir/include/mlir/IR/Builders.h
337

nit: Spell out auto here.

mlir/lib/Dialect/Affine/Transforms/SuperVectorize.cpp
1007

nit: Drop trivial braces here.

aartbik added inline comments.Aug 28 2020, 3:36 PM
mlir/include/mlir/IR/Builders.h
332

This is very nitty, but the comment order does not match the code order.
Would you mind using the same order, it just reads better in my mind....

mlir/lib/Dialect/Affine/Transforms/SuperVectorize.cpp
939

can't we just simply return false right away, and get rid of the local bool altogether?

950

use "auto" less, VectorType and Value are reasonable types here

mlir/test/Dialect/Affine/SuperVectorize/uniform_divergent.mlir
17

hmm, this is a new style to me, would you mind either moving all CHECKs of a method together, or at least move these into the function body itself

(or correct me if I am wrong and point me to some place that uses this style)

dcaballe updated this revision to Diff 289040.Aug 31 2020, 4:25 PM
dcaballe marked 3 inline comments as done.

Thanks! Addressing feedback.

mlir/lib/Dialect/Affine/Transforms/SuperVectorize.cpp
939

Good catch! Thanks!

950

Per LLVM's coding standards (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable):

Don’t “almost always” use auto, but do use auto with initializers like cast<Foo>(...) or other places where the type is already obvious from the context.

This is a bit subjective but IMO these two cases fall into the same category as cast, isa, dyn_cast, since the type is already spelled out in the same line:

VectorType vectorTy = getVectorType...
BroadcastOp bcast = builder.create<BroadcastOp>...

Is MLIR following different guidelines in this regard? Otherwise, I would prefer to stick to the guidelines (whatever they are) since this is one of those topics where every reviewer/developer has a different opinion on :).

mlir/test/Dialect/Affine/SuperVectorize/uniform_divergent.mlir
17

You can find similar tests, for example, here:
mlir/test/Transforms/loop-fusion.mlir
mlir/test/Transforms/buffer-placement.mlir

There are probably more. What is exactly the concern? That LABEL check is separated from the other CHECKs?
For loop transformations I personally think it's important that we keep both the input and the output as clean as possible so that we can clearly see the loop structure of them. Mixing CHECK rules with the input IR wouldn't help in that regard. I can move the LABEL next to the other rules but the LABEL check is usually placed right before the function op since it works as a barrier across tests.

nicolasvasilache accepted this revision.Sep 1 2020, 8:41 AM

Thanks for extending the SuperVectorizer !

This revision is now accepted and ready to land.Sep 1 2020, 8:41 AM

Thanks! Any other comments, @aartbik?

aartbik accepted this revision.Sep 2 2020, 12:24 PM
aartbik added inline comments.
mlir/lib/Dialect/Affine/Transforms/SuperVectorize.cpp
950

Fine with me. Thanks for checking the standard.

mlir/test/Dialect/Affine/SuperVectorize/uniform_divergent.mlir
17

I am fine with this if you prefer. The concern was that breaking the CHECKs this way makes it a bit harder to connect the pieces in your head when reading, but I agree that is subjective.

mehdi_amini added inline comments.Sep 3 2020, 12:04 PM
mlir/include/mlir/IR/Builders.h
343

This API is a bit problematic in the ambiguity it creates for SingleResult operation which implicitly convert to Value as well.

Op op;
builder.setInsertionPointAfter(op);

Will fail after this change.

dcaballe added inline comments.Sep 4 2020, 11:22 AM
mlir/include/mlir/IR/Builders.h
343

Good point! Should we rename it to setInsertionPointAfterValue, for example?