Page MenuHomePhabricator

[NFC][InstructionCost] Refactor LoopVectorizationCostModel::selectVectorizationFactor
AbandonedPublic

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

Details

Summary

This patch is the first of two patches that are designed to migrate
costs in LoopVectorize.cpp to use the new InstructionCost class.
This refactoring is necessary preparation for the second part, which
switches from using float cost to InstructionCost. The tuple is
introduced here because the InstructionCost could be invalid and
so it's not possible to maintain a running cost value in the loop
that divides the cost by the width.

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:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2020, 6:41 AM
sdesmalen added inline comments.Nov 26 2020, 9:14 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5414

std::pair<unsigned, unsigned>

5415

std::pair<unsigned, unsigned> ScalarCost = {ExpectedCost, 1}

5422

std::numeric_limits<unsigned>::max() ?

5435

std::pair<unsigned, unsigned> (that also allows removing the cast on line 5461)

david-arm added inline comments.Nov 27 2020, 12:43 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5414

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.

david-arm added inline comments.Nov 27 2020, 12:56 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5414

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.

sdesmalen added inline comments.Nov 27 2020, 2:00 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5414

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.

david-arm added inline comments.Nov 27 2020, 2:36 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5414

Ah yes I see, that would work. I'll update the patch!

david-arm updated this revision to Diff 308013.Nov 27 2020, 3:55 AM
  • Changed float->unsigned in the tuple used for selecting vectorisation factors.
sdesmalen added inline comments.Nov 27 2020, 7:48 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5415

I think you missed this one. This would remove the need for {ScalarCost, 1} on line 5456.

5437

nit: cast is no longer necessary?

5460

nit: return {ElementCount::getFixed(MinCost.second), MinCost.first};

david-arm updated this revision to Diff 308297.Nov 30 2020, 2:50 AM
david-arm marked 7 inline comments as done.
ctetreau added inline comments.Nov 30 2020, 7:32 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5414

Why was this removed?

david-arm added inline comments.Nov 30 2020, 7:36 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5414

Not sure which line you're referring to here - the debug line is still there I think?

I don't think you needs these pairs. If you check validity of the InstructionCost in the for loop, and break or continue if it's invalid, then you can just use a scalar value as it did before. (I commented as such on D92178)

Since this patch just adds the pair, you should be able to abandon it (unless I'm missing something)

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5414

ahh, I see it now. The diff has it in all-red, but it looks like you just shuffled some lines.

david-arm abandoned this revision.Dec 18 2020, 12:46 AM

After some recent upstream discussions in the SVE sync call this patch is no longer needed.