[PATCH] D88962: [SVE] Add support for scalable vectors in vectorize_width loop attribute

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/docs/LangRef.rst:5893
 
+In the second format we can write the attribute as follows:
+
----------------
Rather than talking about two forms, is it sufficient to say:

```The vector width is an ElementCount tuple, represented in Metadata as:

.. code-block:: llvm

   !0 = !{!"llvm.loop.vectorize.width", !1}
   !1 = !{i32 4, i32 1}

where ``i32 4`` specifies the vector width and ``i32 1`` indicates if the vectorization factor is scalable, meaning that the loop-vectorizer should use vector-length agnostic vectorization.

For fixed-width vectorizatoin-factors, a short-hand `i32` operand for llvm.loop.vectorize.width is also supported.

.. code-block:: llvm
    !0 = !{!"llvm.loop.vectorize.width", i32 4}
```


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:67
+      return false;
+    const ConstantInt *C0 =
+        mdconst::dyn_extract<ConstantInt>(MD->getOperand(0));
----------------
david-arm wrote:
> fhahn wrote:
> > are you anticipating additional hints with ElementCount metadata? If not, it might be simpler to just deal with HK_WIDTH up front and validate Val[0] and Val[1] here and leave the code handling the other cases mostly unchanged? Or maybe use named variables instead of an array, to make things a bit clearer?
> So the reason I structured it like this is because we are still maintaining backwards compatibility with the old style and allowing single integer constants instead of the node. I could deal with the HK_WIDTH up front, but I'd still need all this code. Would you be ok with using named variables instead?
nit: For the cases below, is it worthing using something like:
```
auto MaySetIntValue = [this](int IntVal, bool Condition) { if (Condition) this->Value.U32 = IntVal; return Condition; };
auto MaySetECValue = [this](ElementCount EC, bool Condition) { if (Condition) this->Value.EC = EC; return Condition; };

switch (Kind) {
case HK_WIDTH:
  return MaySetECValue(ElementCount::get(IntVal, IsScalable), isPowerOf2_32(IntVal) && IntVal <= VectorizerParams::MaxVectorWidth);
case HK_UNROLL:
  return MaySetIntValue(IntVal, isPowerOf2_32(Val) && Val <= MaxInterleaveFactor);
[...]
}
```


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88962/new/

https://reviews.llvm.org/D88962