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

[Caroline via Phabricator via llvm-commits]

CarolineConcatto added a comment.

Hi @ctetreau,
Thank you for the review.
So I removed all the asserts and added the earlier return if both costs are invalid, because in this case it means that the transformation is not cheap.
If only OldCost is invalid I believe we should do nothing, for the same reason I removed the test for !NewCost.isValid().



================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:250
 
+  assert((Cost0.isValid() || Cost1.isValid()) &&
+         "At least one of Cost0 or Cost1 should be valid");
----------------
ctetreau wrote:
> Reasonable to return nullptr?
> 
> If neither cost is valid, then neither of the inputs should be replaced
I think it is fine to add if the test if both are equal following the logic of  if (Index0 == Index1)


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:358-359
 
+  assert((OldCost.isValid() || NewCost.isValid()) &&
+         "At least one of OldCost or NewCost should be valid");
   // Aggressively form a vector op if the cost is equal because the transform
----------------
ctetreau wrote:
> Would it be reasonable to return false here? If all costs involved are invalid, then I would say the transform is not cheap.
If I remove the assert and leave the test to do its job the return will be false, because OldCost would be equal to NewCost.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:637
   // We want to scalarize unless the vector variant actually has lower cost.
+  assert(OldCost.isValid() && "Invalid initial cost");
   if (OldCost < NewCost)
----------------
ctetreau wrote:
> reasonable to return false?
If both are invalid, maybe. 
But if OldCost is invalid I think you mean return true, because OldCost>NewCost
And in this last case I don't think we should add the earlier return because it is a change on the code path.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:739
   // Codegen can reverse this transform (scalarize) if it was not profitable.
+  assert(OldCost.isValid() && "Invalid initial cost");
   if (OldCost < NewCost)
----------------
ctetreau wrote:
> reasonable to return false?
If both are invalid, maybe. 
But if OldCost is invalid I think you mean return true, because OldCost>NewCost
And in this last case I don't think we should add the earlier return because it is a change on the code path.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94069