[PATCH] D92177: [NFC][InstructionCost] Refactor LoopVectorizationCostModel::selectVectorizationFactor

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5414
+
+  std::pair<float, unsigned> MinCost = {ExpectedCost, 1};
+  const float ScalarCost = ExpectedCost;
----------------
sdesmalen wrote:
> david-arm wrote:
> > david-arm wrote:
> > > sdesmalen wrote:
> > > > `std::pair<unsigned, unsigned>` 
> > > This is no longer a NFC change if I use unsigned. This changes the behaviour of cases where different vectorisation factors produce fractional, and different, costs. For example, (float) 10 / 2 is less than (float) 11 / 2, whereas using integers would give the same result of 5. I don't mind making the change, but then I think I should remove NFC from the subject line.
> > I think what I can do keep it as NFC is simultaneously change float->unsigned and change the algorithm in isLowerVectorCost() to do the same as the dependent patch - D92178. That way we'd still make the same choice of VF as before.
> Perhaps I misunderstood, but I thought the reason for having this as a `std::pair` is so that you can postpone the division itself, i.e. the MinimumCost is `ExpectedCost / 1`, and later on as `VectorCost / i`, where the MinimumCost variable itself is represented as `{ExpectedCost, 1}` and `{VectorCost, i}`. You'd need to add a cast in `isLowerVectorCost` to make sure both LHS and RHS of the comparison are evaluated as `float`.
Ah yes I see, that would work. I'll update the patch!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92177