[PATCH] D87700: [SVE] Replace *, / operators in TypeSize/ElementCount with mul, div

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:60
 
-  ElementCount &operator*=(unsigned RHS) {
-    Min *= RHS;
-    return *this;
+  ElementCount mul(unsigned RHS) const {
+    return ElementCount(Min * RHS, Scalable);
----------------
As we discussed in the SVE sync-up call, `mul` isn't strictly needed but rather added for consistency.
That raises the question whether `operator-` and `operator+` also need to have a `sub` and `add` functions, as those are not "mathematically correct" either given they only support adding either 2 scalable, or 2 fixed-sized sizes, and they assert otherwise.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:188
 
-  TypeSize operator/(unsigned RHS) const {
-    return { MinSize / RHS, IsScalable };
-  }
+  TypeSize div(uint64_t RHS) const { return {MinSize / RHS, IsScalable}; }
 
----------------
Given the particular reasons for adding these in favour of the overloaded operators, these functions need a clear description that explains why we've done so, and why the results may not be entirely arithmatically correct when uses of this function assume vscale to be some specific value at compile-time.



================
Comment at: llvm/include/llvm/Support/TypeSize.h:241
 
+  bool isKnownMultipleOf(uint64_t RHS) const { return MinSize % RHS == 0; }
+
----------------
Please add a description for this method.


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

https://reviews.llvm.org/D87700