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

[Francesco Petrogalli via Phabricator via llvm-commits]

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)
----------------
fpetrogalli wrote:
> 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.
Ah, `llvm::SmallSetVector`! I think I am going to try that first, so we don't need both ordering function and the STL.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85794