[PATCH] D95803: Ensure that InstructionCost actually implements a total ordering

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Thanks for fixing this @ctetreau, I guess this becomes more important when InstructionCosts are used in sets that rely on a total ordering in order to get bit-reproducability between builds.
Just added a few nits, but looks fine to me otherwise.



================
Comment at: llvm/include/llvm/Support/InstructionCost.h:151
+  /// ordering where valid costs are always considered to be less than invalid
+  /// costs. This avoids having to add asserts the comparison operators that the
+  /// states are valid and users can test for validity of the cost explicitly.
----------------
nit: `s/ / to /`


================
Comment at: llvm/include/llvm/Support/InstructionCost.h:154-157
+    if (State < RHS.State)
+      return true;
+
+    return Value < RHS.Value;
----------------
nit: `return State < RHS.State || Value < RHS.Value;` ?


================
Comment at: llvm/include/llvm/Support/InstructionCost.h:161
   bool operator==(const InstructionCost &RHS) const {
-    return State == RHS.State && Value == RHS.Value;
+    return !(*this < RHS) && !(RHS < *this);
   }
----------------
nit: Is it worth adding a comment why this is implement in such an unusual way, so that the reason for that doesn't get lost in the future (it is mentioned in the commit message, but that's easily ignored)


================
Comment at: llvm/unittests/Support/InstructionCostTest.cpp:42
 
+  EXPECT_TRUE((IThreeA < ITwo) || (IThreeA > ITwo) || (IThreeA == ITwo));
+  EXPECT_NE(IThreeA, ITwo);
----------------
Is there value in this test, if all three subexpressions have their expected value tested below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95803