[PATCH] D32019: [MVT][SVE] Scalable vector MVTs (3/3)

[Graham Hunter via Phabricator via llvm-commits]

huntergr added inline comments.


================
Comment at: include/llvm/CodeGen/MachineValueType.h:241
+    public:
+      uint64_t Min;
+      bool Scalable;
----------------
RKSimon wrote:
> Why uint64_t and not unsigned like the other existing vector element count uses?
It was mostly to match the equivalent for IR -- VectorType::getNumElements() returns a uint64_t now (since NumElements was moved to SequentialType). I guess it doesn't need to be the same for CodeGen.


================
Comment at: include/llvm/CodeGen/MachineValueType.h:607
 
+    MVT::ElementCount getVectorElementCount() const {
+      return { getVectorNumElements(), isScalableVector() };
----------------
rengolin wrote:
> Can't you just return EC here?
There isn't a defined EC variable for an MVT; it just contains a single SimpleValueType. Each MVT used would be noticeably larger (and increasing from 8bits to 16bits may already cause some problems, see comments on patch 2) if it included an ElementCount instance, so we just construct them on the fly based on the enum value of SimpleTy.


================
Comment at: include/llvm/CodeGen/ValueTypes.h:77
+      // We don't support extended scalable types yet.
+      assert(!IsScalable);
       return getExtendedVectorVT(Context, VT, NumElements);
----------------
rengolin wrote:
> Add the comment inside the assert, like:
> 
>     assert(!IsScalable && "we don't support extended scalable types yet");
Ok, will do.


================
Comment at: include/llvm/CodeGen/ValueTypes.h:157
+      // We don't support extended scalable types yet.
+      if (!isSimple())
+        return false;
----------------
rengolin wrote:
> I don't think this check is necessary, and may bite you later if you forget.
> 
> Either make it an assert, like the others, or just let the comparison get inlined.
So I would have preferred to use an assert, but this is called by existing code on extended types because the asserts query it.

This check and the asserts can all be removed once the IR type is in place.


https://reviews.llvm.org/D32019