[PATCH] D85794: [WIP][llvm][LV] Replace `unsigned VF` with `ElementCount VF` [NFCI]

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli marked an inline comment as done.
fpetrogalli added inline comments.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:63
+  /// Ordering function for ElementCount, needed for `set` like containers.
+  bool operator<(const ElementCount &EC) const {
+    if (this->Scalable == EC.Scalable)
----------------
ctetreau wrote:
> I'm uncomfortable with having this comparitor. is `{true, 4} > {false, 4}`? Depends on the machine. I would personally prefer a named function that can be passed to the comparator parameter of containers. I understand that this makes the code uglier, so if you get significant pushback it may be worth caving on this issue.
I think what you say makes sense.  I had to introduce a comparison operator because `llvm::SmallSet` is being used in VPlan. 

An alternative approach would be to remove the small set and have a hash function that could be used for std::unordered_set. I actually prefer this latter approach as it avoid introducing the concept of ordering ElementCount, which is kind of formally wrong as you have pointed out.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:87
+  /// One or more elements.
+  bool isVector() const {
+    assert(!(Min == 0 && Scalable) &&
----------------
ctetreau wrote:
> Constraining the constructor doesn't help with the case where a valid `ElementCount` is constructed, and then `Min` set to zero later on.
> 
> Why exactly is `{true, 0}` forbidden? `forall N, N * 0 = 0` after all.

Mathematically you are right. ElementCount is  a monomial of grade 0 (a*X^0) or 1 (a*X^1). It doesn't really matter if you are multiplying by 0*X^0 or 0*X^1, both values will null the result of the operation independently of the EC in input.

I think that @vkmr was mostly concerned about potential bugs that can happen when we check if we are working in scalable mode (so Scalable == true) but we forget to check that we have actually a non zero value by checking Min > 0. It is very unlikely to happen, but I guess there is some argument for choosing this approach. 

All in all, I'd rather avoid `{0, true}`, at list in debug mode, just to catch things that might end up weird if we end up producing this value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794