[PATCH] D88098: [SVE] Add new isKnownXX comparison functions to TypeSize

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:176
+
+  static bool isKnownLT(const TypeSize &LHS, const TypeSize &RHS) {
+    if (!LHS.IsScalable || RHS.IsScalable)
----------------
sdesmalen wrote:
> Sorry to spot this only after you've landed the patch, but was there an explicit reason for making these functions `static` as opposed to allowing the shorter `X.isKnownLT(Y)` (that wouldn't need the `TypeSize::` or `ElementCount::` prefix)?
So it just seemed like a much more natural replacement for the existing operators, i.e. friend functions with a LHS and RHS:

friend bool operator<(const TypeSize &LHS, const TypeSize &RHS)

Also, I can see value in introducing the operators this way as in future once we have a templated PolySize we might want these operators to be more generic, such as:

template <typename X, typename Y>
bool SomeNameSpace::isKnownGT(const PolySize<X> LHS, const PolySize<Y> RHS);

or even have the LHS not be a PolySize at all (i.e. LHS is a simple integer):

template <typename X, typename Y>
bool SomeNameSpace::isKnownGT(X LHS, const PolySize<Y> RHS) {
  PolySize<X> LHS2 = PolySize::getFixed(LHS);
  return isKnownGT(LHS2, RHS);
}

and so on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88098