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

[Paul Walker via Phabricator via llvm-commits]

paulwalker-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:
> 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.
To be clear I have nothing against relying on the total ordering but I feel if the transformation is expecting instructions to have an actual cost then it should either assert or explicitly check for such.  An example of this is LoopVectorize where there has already been extensive validation to ensure a loop is vectorisable and thus not being able to cost the loop is a sure sign there's either a bug in LoopVectorize's isLegal code or the cost functions themselves.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94069