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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau 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)
----------------
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.


================
Comment at: llvm/include/llvm/Support/TypeSize.h:87
+  /// One or more elements.
+  bool isVector() const {
+    assert(!(Min == 0 && Scalable) &&
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794