This is an archive of the discontinued LLVM Phabricator instance.

[NFC][InstructionCost] Change LoopVectorizationCostModel::getInstructionCost to return InstructionCost
ClosedPublic

Authored by david-arm on Nov 26 2020, 6:42 AM.

Details

Summary

This patch is part of a series of patches that migrate integer
instruction costs to use InstructionCost. In the function
selectVectorizationFactor I have simply asserted that the cost
is valid and extracted the value as is. In future we expect
to encounter invalid costs, but we should filter out those
vectorization factors that lead to such invalid costs.

See this patch for the introduction of the type: https://reviews.llvm.org/D91174
See this thread for context: http://lists.llvm.org/pipermail/llvm-dev/2020-November/146408.html

Diff Detail

Event Timeline

david-arm created this revision.Nov 26 2020, 6:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2020, 6:42 AM
david-arm retitled this revision from [InstructionCost] Migrate to using InstructionCost in LoopVectorize.cpp to [InstructionCost] Change LoopVectorizationCostModel::getInstructionCost to return InstructionCost.Nov 26 2020, 9:01 AM
david-arm updated this revision to Diff 308014.Nov 27 2020, 3:56 AM
  • Rebased off parent patch.
david-arm updated this revision to Diff 308299.Nov 30 2020, 2:52 AM
ctetreau added inline comments.Nov 30 2020, 9:56 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5785–5786

I think you can get rid of these pairs if you just check validity before assigning it.

5789

ForceVectorization needs to not blow up in the face of invalid InstructionCosts

david-arm added inline comments.Dec 1 2020, 6:01 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5785–5786

OK. I guess the problem is that we could start off with an invalid cost (ScalarCost) and get another Invalid cost in the loop too. I think we could get rid of the pairs, but it just makes the main loop more complicated that's all. What I could do is add this in the main loop:

if (!C.first.isValid())
  continue
if (!ScalarCost.isValid())
  MinCost = C.first;
// Both costs are now valid.
// ... use MinCost and C somehow ...

The other thing I was trying to do is move from using a float to represent the MinCost in the loop, to using an InstructionCost (integer based) instead. If I get rid of the pairs then it makes more sense to revert to using a float for MinCost I think, since then division (and hence a fractional cost) is involved.

I can try adding control flow to the loop like above?

ctetreau added inline comments.Dec 1 2020, 8:39 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5785–5786

So I messed around with your patch, and came up with this loop body:

for (unsigned i = 2; i <= MaxVF.getFixedValue(); i *= 2) {
    // Notice that the vector loop needs to be executed less times, so
    // we need to divide the cost of the vector loops by the width of
    // the vector elements.
    VectorizationCostTy C = expectedCost(ElementCount::getFixed(i));

    Optional<float> VectorCost =
        C.first.getValue().map([i](InstructionCost::CostType Cost) {
          return static_cast<float>(Cost) / i;
        });

    // removed debug output noise

    if (auto MinCostVal = MinCost.getValue())
      if (VectorCost && *VectorCost < *MinCostVal) {
        Width = i;
        // requires ctor that takes an Optional, and ctors that convert from number like things
        MinCost = VectorCost;
      }
  }

I haven't ran the tests, but this compiles and I think it should work.

david-arm added inline comments.Dec 2 2020, 6:29 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5785–5786

Hi @ctetreau, your suggestions for adding a conversion constructor/getter in general seem sensible to me, however I just have one question. In that example above it looks like you're rounding a float (VectorCost) to an integer, which is a change in behaviour I think? Currently the examples I'm thinking of are where we compare a cost of 11 for a width of 2, with a cost of 21 for a width of 4, i.e.

float(11)/2 = 5.5
float(21)/4 = 5.25

Previously we'd choose a width of 4 because 5.25 is less than 5.5. I think with your proposal we'd keep a width of 2 because 5.5 gets rounded down to 5. Unless we make MinCost use floats too, in which case the need for conversion goes away I think?

ctetreau added inline comments.Dec 3 2020, 5:23 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5785–5786

Yeah, looks like I messed up. If you have MinCost be an Optional<float>, it seems like it should be fine. The main idea is that this whole function should be working with Optional<float> instead of InstructionCost. We can convert all InstructionCost objects to Optional<float> right away, do floating point math with them, and then convert back to unsigned for the return.

david-arm retitled this revision from [InstructionCost] Change LoopVectorizationCostModel::getInstructionCost to return InstructionCost to [NFC][InstructionCost] Change LoopVectorizationCostModel::getInstructionCost to return InstructionCost.
david-arm edited the summary of this revision. (Show Details)
david-arm updated this revision to Diff 313040.Dec 21 2020, 1:56 AM
  • Removed assert in Instruction::ExtractValue case in getInstructionCost.
  • Rebase.

Hi folks, just a gentle ping to see if anyone has chance to have a quick look?

LGTM, but it's not clear to me if @ctetreau's latest comment still needs to be addressed.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5764–5767

nit: ExpectedScalarCost ?

david-arm marked 4 inline comments as done.Jan 8 2021, 5:56 AM
david-arm added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5785–5786

Hi @ctetreau, I've changed my patch now to avoid worrying about validity of the cost. Since we now assert the cost is valid we no longer need to use Optionals and so on.

sdesmalen accepted this revision.Jan 8 2021, 6:21 AM
This revision is now accepted and ready to land.Jan 8 2021, 6:21 AM