[PATCH] D66871: [SVE] MVT scalable size queries

Thu Aug 29 06:34:21 PDT 2019

huntergr added a comment.

In D66871#1649148 <https://reviews.llvm.org/D66871#1649148>, @cameron.mcinally wrote:

> LGTM

Thanks for the review; there's still D53137 <https://reviews.llvm.org/D53137> and D66339 <https://reviews.llvm.org/D66339> as prerequisites so I won't commit this yet.

================
Comment at: llvm/include/llvm/CodeGen/ValueTypes.h:292
+    /// Return the size of the specified value type in bits. An assert will
+    /// occur if this is called on a scalable vector type.
     unsigned getSizeInBits() const {
----------------
cameron.mcinally wrote:
> Nit-picky: would it make sense to add an explicit !scalable assert to all these functions?
> 
> I see that this particular function is guarded by the asserts in getSizeInBits() and getExtendedSizeInBits(), but there's really no guarantee that future modifications will preserve this behavior.
> 
> To be clear, I don't feel strongly about this. Just thinking aloud...
I can certainly add asserts more widely to make it obvious instead of relying on lower functions to assert and just commenting on it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D66871