[PATCH] D94069: [NFC][InstructionCost]Migrate VectorCombine.cpp to use InstructionCost

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:252
+  // and consider the other too expensive
+  assert((Cost0.isValid() && Cost1.isValid()) && "Invalid Cost0 or Cost1");
+  if (!Cost0.isValid() && Cost1.isValid())
----------------
david-arm wrote:
> ctetreau wrote:
> > Because of this assert, the branches will never be taken. This will result in different behavior in release vs debug mode.
> > 
> > Either remove the assert, or remove the two early returns.
> > 
> > Regardless, invalid costs are guaranteed to compare higher than valid costs, so the early returns are redundant.
> Hi @ctetreau, after discussion during the last SVE sync call @paulwalker-arm thought we shouldn't be relying upon the lexicographical ordering that defines invalid costs to be infinitely expensive. He suggested that doing so is actually a bug in the code. So the route we've been taking so far is to either check for validity explicitly or assert that it's valid. If you think this is the wrong approach here then we can perhaps discuss it and agree on a consistent approach?
I'm not sure how I missed that conversation, but as you may recall from the review thread for D91174, I fought hard for the total ordering to be added and documented so that it's guaranteed to be true. This is exactly the sort of case you'd want to this ordering for; the validity checks are guaranteed to be redundant with the greater-than checks. Additionally, adding the redundant validity checks is more error prone, because it's more operator-heavy lines of code you can mess up.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94069