Page MenuHomePhabricator

[LV] Fix scalar cost for tail predicated loops
AcceptedPublic

Authored by dmgreen on Aug 24 2020, 6:06 AM.

Details

Summary

When it comes to the scalar cost of any predicated block, the loop vectorizer by default regards this predication as a sign that it is looking at an if-conversion and divides the scalar cost of the block by 2, assuming it would only be executed half the time. This however makes no sense if the predication has been introduced to tail predicate the loop.

Diff Detail

Event Timeline

anwel created this revision.Aug 24 2020, 6:06 AM
anwel requested review of this revision.Aug 24 2020, 6:06 AM
fhahn added a comment.Aug 24 2020, 6:13 AM

Can you add the test separately and then just have the diff of the cost-model change in this patch?

llvm/test/Transforms/LoopVectorize/ARM/scalar-block-cost.ll
1

Are the lines actually auto-generated? Looks like they are just a subset of the debug output?

8

nit: are dse_local, no capture, read none local_unnamed_addr actually needed?

103

are all those attributes needed? can you limit them to the minimum required?

anwel updated this revision to Diff 287391.Aug 24 2020, 7:38 AM
anwel marked 3 inline comments as done.

Thanks for the suggestions, I cleaned up the test so it looks a bit nicer now.

llvm/test/Transforms/LoopVectorize/ARM/scalar-block-cost.ll
1

That's true. A dumb copy-paste mistake, sorry about that and thanks for catching.

8

Not really, no.

103

I can, only the target-features are needed.

@dmgreen Since Anna is no longer with us, are you going to commander this?

Yep, that's the plan. It will need a number of other patches though. I think one for adding a cost to predicated blocks is important, along with the one for costing masked loads more correctly under MVE.

Okay, cheers.

fhahn accepted this revision.Oct 5 2020, 4:26 AM

LGTM, thanks!

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

might be worth adding a note here that we do intentionally not use LoopVectorizationCostModel::blockNeedsPredication, this also returns true if predication is required due to folding the tail by masking.

This revision is now accepted and ready to land.Oct 5 2020, 4:26 AM
dmgreen commandeered this revision.Oct 13 2020, 7:26 AM
dmgreen edited reviewers, added: anwel; removed: dmgreen.

Thanks. We need to be careful with this as it has the potential to cause some regressions, especially as we here (in MVE land) make heavy use of predication nowadays. I am working through some of the details.