Page MenuHomePhabricator

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

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



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

nit: Spell out auto here.


nit: Drop trivial braces here.

aartbik added inline comments.Aug 28 2020, 3:36 PM

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....


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


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


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.


Good catch! Thanks!


Per LLVM's coding standards (

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 :).


You can find similar tests, for example, here:

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.

Fine with me. Thanks for checking the standard.


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

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

Op op;

Will fail after this change.

dcaballe added inline comments.Sep 4 2020, 11:22 AM

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