[PATCH] D27101: [Type] Extend VectorType to support scalable vectors. [IR support for SVE scalable vectors 1/4]

[Renato Golin via Phabricator via llvm-commits]

rengolin added a comment.

Hi Paul,

I want to restart the scalable vector discussion. My original inline comments are still valid and I have a few more.

I think the general idea is sound and in line with a generic "scalable" vector, as well as with how SVE vectors should be represented.

cheers,
--renato



================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2125
+    case bitc::TYPE_CODE_VECTOR: {  // VECTOR: [scalable, numelts, eltty]
+      unsigned Size = Record.size();
+      if (Size < 2)
----------------
I think this would be much clearer if you stick to 0, 1, 2 indexes rather than "Size - x". We guarantee (in the language) that size is either 2 or 3 or invalid.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:2133
         return error("Invalid type");
-      ResultTy = VectorType::get(ResultTy, Record[0]);
+      bool Scalable = Size > 2 ? Record[Size - 3] : false;
+      ResultTy = VectorType::get(ResultTy, Record[Size - 2], Scalable);
----------------
Why "Record[Size - 3]"? I'd say...

    bool Scalable = Size > 2 ? true : false;

as guaranteed by the language reference (and your comment in `DerivedTypes.h`.


https://reviews.llvm.org/D27101