[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:201
+  assert(OldCost.isValid() && "Invalid initial cost");
+  if (OldCost < NewCost || !NewCost.isValid())
     return false;
----------------
the or is redundant. 


================
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())
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:325
+  InstructionCost CheapExtractCost =
+      InstructionCost::min(Extract0Cost, Extract1Cost);
 
----------------
can std::min be used here? InstructionCost has overloaded comparison operators and a total ordering.

Assuming it can be, we should probably get rid of InstructionCost::min and InstructionCost::max. That can be a different patch.


================
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
----------------
this is redundant. If OldCost is valid, and NewCost is invalid, then OldCost < NewCost returns true.


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:537
+  assert(SrcCost.isValid() && "Invalid source cost");
+  if (DestCost > SrcCost || !DestCost.isValid())
     return false;
----------------
the or is redundant


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:648
+  assert(OldCost.isValid() && "Invalid initial cost");
+  if (OldCost < NewCost || !NewCost.isValid())
     return false;
----------------
the or is redundant


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:750
+  assert(OldCost.isValid() && "Invalid initial cost");
+  if (OldCost < NewCost || !NewCost.isValid())
     return false;
----------------
the or is redundant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94069