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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau 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);
----------------
sdesmalen wrote:
> 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.
I think given the outcome of the call today, operator* can probably stay.

If we're going to document the header that "if it behaves the same as in real math, then it's an operator. If not, it's a named function", then this multiply is fine.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:64
 
-  ElementCount &operator/=(unsigned RHS) {
-    Min /= RHS;
-    return *this;
+  ElementCount div(unsigned RHS) const {
+    return ElementCount(Min / RHS, Scalable);
----------------
I would suggest renaming this to something more self-documenting. Perhaps `coefficientDiv`? This has the added benefit of making it seem less strange that it's a named divide rather than operator/


================
Comment at: llvm/include/llvm/Support/TypeSize.h:186-188
+  TypeSize mul(uint64_t RHS) const { return {MinSize * RHS, IsScalable}; }
 
+  TypeSize div(uint64_t RHS) const { return {MinSize / RHS, IsScalable}; }
----------------
Same comment as ElementCount


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

https://reviews.llvm.org/D87700