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

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

Added two nits, but LGTM otherwise.



================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:635
   // We want to scalarize unless the vector variant actually has lower cost.
-  if (OldCost < NewCost)
+  if (OldCost < NewCost || (!OldCost.isValid() && !NewCost.isValid()))
     return false;
----------------
nit: `|| !NewCost.isValid()` should be sufficient (see the other comment).


================
Comment at: llvm/lib/Transforms/Vectorize/VectorCombine.cpp:750
+  assert(OldCost.isValid() && "Invalid initial cost");
+  if (OldCost < NewCost || !NewCost.isValid())
     return false;
----------------
ctetreau wrote:
> the or is redundant
I don't think the `or` was redundant.

If the new cost is invalid, then it shouldn't do the transform. With only testing `OldCost < NewCost`, we'd get:
```
        isValid?
   OldCost  NewCost       (OldCost < NewCost)     result
--------------------------------------------------------
1.    true    true     OldCost.Val < NewCost.Val    ?
2.    true    false          Valid < Invalid       true
3.    false   true         Invalid < Valid        false
4.    false   false        Invalid < Invalid      false

However, 4. should be 'true' in order to return early from the function.

         isValid?
   OldCost  NewCost  (OldCost < NewCost || !NewCost.isValid)    result
-----------------------------------------------------------------------
1.    true    true      OldCost.Val < NewCost.Val || false        ?
2.    true    false           Valid < Invalid || true            true
3.    false   true          Invalid < Valid   || false          false
4.    false   false         Invalid < Invalid || true            true

Gives us the result we want.
```
nit: based on that I believe `!OldCost.isValid() && ` is now redundant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94069