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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

In D94069#2489637 <https://reviews.llvm.org/D94069#2489637>, @CarolineConcatto wrote:

> Also, thank you for making clear that invalid, atm, means as well high cost.
> I'll have that in mind for the next patches.

I would say "infinitely costly", not "high cost". Somebody may have "a lot" of LLVMBucks, nobody has infinity LLVMBucks.

Sorry to nitpick, but it's important to get these things right initially before everybody sees some flawed example and emulates it. If you need something to have a really high cost, you should just pick some really high valid cost. If you need something to never be within any cost budget, you should use invalid.



================
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())
----------------
sdesmalen wrote:
> paulwalker-arm wrote:
> > 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.
> It seems that the algorithm requires at least one of the Costs to be valid (it has to choose either Ext0 or Ext1), so if the assert is changed to:
> 
>   assert((Cost0.isValid() || Cost1.isValid()) && "At least one of Cost0 and Cost1 should be valid");
> 
> the existing code below should be sufficient and the two early returns that were added can be removed like @ctetreau suggested.
> 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.

If it's an honest-to-gosh bug for some call to return invalid, then this is fine. I feel like this should never happen in any function that returns `InstructionCost` though. This would be akin to swallowing an exception and calling `exit()`.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94069