This is an archive of the discontinued LLVM Phabricator instance.

[LV] Fix scalar cost for tail predicated loops
ClosedPublic

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
3

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.

This revision was automatically updated to reflect the committed changes.

@dmgreen While I'm not sure this patch is a real root cause of the loop vectorizer crash https://bugs.llvm.org/show_bug.cgi?id=48564 but revert of this patch eliminates the crash.
I would appreciate if you could look into the bug.

Will do. I expected some fallout from this one.

Thanks for the report. Ill try and take a look.

Will do. I expected some fallout from this one.

Thanks for the report. Ill try and take a look.

Thanks a lot!