[PATCH] D87700: [SVE] Replace / operator in TypeSize/ElementCount with coefficientDiv

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm added a comment.

I'm not a great fan of the coefficientDiv name but once in I doubt I'll give it a second thought.  That said, because by far the most common usage is coefficientDiv(2), whose intent is clearly to knowingly split something into two equal parts I'm wondering if adding a `halve()` utility function will keep the main usage short and meaningful and then there's even less reason to care about the name of coefficientDiv.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:86-89
+  /// compile time to be a multiple of the scalar value RHS. If the element
+  /// count is not scalable then a false return value guarantees it is not
+  /// a multiple of RHS. However, for scalable values a false return simply
+  /// means we don't know, since this depends on the value of 'vscale'.
----------------
I don't think it's a good idea to promote incompatible usages of this function.  Let's keep the messaging simple with only a true result being something that is meaningful, which is inline with the other isKnown methods.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:278-281
+  /// compile time to be a multiple of the scalar value RHS. If the type
+  /// size is not scalable then a false return value guarantees it is not
+  /// a multiple of RHS. However, for scalable values a false return simply
+  /// means we don't know, since this depends on the value of 'vscale'.
----------------
As above.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19500
       unsigned DestSrcRatio = DestNumElts / SrcNumElts;
       if ((NVT.getVectorMinNumElements() % DestSrcRatio) == 0) {
+        ElementCount NewExtEC =
----------------
Should this use isKnownMultipleOf to match the coefficientDiv you've introduced?


================
Comment at: llvm/lib/CodeGen/TargetLoweringBase.cpp:835-836
+      return LegalizeKind(
+          LA, EVT::getVectorVT(Context, SVT.getVectorElementType(),
+                               SVT.getVectorElementCount().coefficientDiv(2)));
     if (LA == TypeScalarizeVector)
----------------
SVT.getHalfNumVectorElementsVT(Context) ?


================
Comment at: llvm/unittests/CodeGen/ScalableVectorMVTsTest.cpp:64
 
   // Check that overloaded '*' and '/' operators work
   EXPECT_EQ(EVT::getVectorVT(Ctx, MVT::i64, EltCnt * 2), MVT::nxv4i64);
----------------
Comment needs updating, I'd just drop the `overloaded '*' and '/'` part.


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

https://reviews.llvm.org/D87700