Page MenuHomePhabricator

[LoopCacheAnalysis]: Fix assertion failure during cost computation
ClosedPublic

Authored by rcraik on Nov 13 2019, 10:30 AM.

Details

Summary

During calculation of the reference cost, possible type differences between the Stride and TripCount are not taken into account, which can result in assertion failure in ScalarEvolution::getMulExpr that the operand types do not match.

This patch addresses this issue.

Diff Detail

Event Timeline

rcraik created this revision.Nov 13 2019, 10:30 AM
bmahjour added inline comments.Nov 13 2019, 11:44 AM
llvm/lib/Analysis/LoopCacheAnalysis.cpp
287

const Type *WiderType?

llvm/test/Analysis/LoopCacheAnalysis/PowerPC/compute-cost.ll
3

A comment saying what the test is testing would be useful.

11

are dso_local, nocapture, local_unnamed_addr necessary?

rcraik marked an inline comment as done.Nov 13 2019, 1:10 PM
rcraik added inline comments.
llvm/lib/Analysis/LoopCacheAnalysis.cpp
287

ScalarEvolution::getNoopOrAnyExtend does not take a constant type

rcraik updated this revision to Diff 229170.Nov 13 2019, 1:17 PM

Updated to address comments from @bmahjour in relation to the testcase

rcraik marked 2 inline comments as done.Nov 13 2019, 1:18 PM
jdoerfert accepted this revision.Nov 13 2019, 4:05 PM

Before I saw that the cost has to be a constant I wrote this:

I'm unsure if a sign extend would not be more appropriate for the stride (for the trip count it doesn't matter I guess), but at the end of the day this is "just" a heuristic.
Feel free to update it to sign extend if you feel that it is appropriate. Maybe try out with a negative stride test if you can see a difference.

Given that it has to be a constant it does not matter. If that might change, consider my comment above.

Otherwise, LGTM.

This revision is now accepted and ready to land.Nov 13 2019, 4:05 PM
rcraik updated this revision to Diff 229609.Nov 15 2019, 11:46 AM

Update Stride to use sign extension

This revision was automatically updated to reflect the committed changes.