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

[David Sherwood via Phabricator via llvm-commits]

david-arm added a subscriber: paulwalker-arm.
david-arm 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())
----------------
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?


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:368-369
+  // is the only valid option
+  if (!NewCost.isValid())
+    return true;
   // Aggressively form a vector op if the cost is equal because the transform
----------------
ctetreau wrote:
> this is redundant. If OldCost is valid, and NewCost is invalid, then OldCost < NewCost returns true.
Again, @CarolineConcatto is just adding checks here as per discussion on the last SVE sync call, but we're happy to discuss the correct approach.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94069