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

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: include/llvm/CodeGen/MachineValueType.h:607
 
+    MVT::ElementCount getVectorElementCount() const {
+      return { getVectorNumElements(), isScalableVector() };
----------------
huntergr wrote:
> 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.
Right, makes sense.


================
Comment at: include/llvm/CodeGen/ValueTypes.h:157
+      // We don't support extended scalable types yet.
+      if (!isSimple())
+        return false;
----------------
huntergr wrote:
> 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.
I see.

Asserts for unsupported things are known to be ephemeral, but this check isn't. Adding a FIXME with that rationale would make it so, for now.


https://reviews.llvm.org/D32019