Page MenuHomePhabricator

[SCEV] Add missing cache queries
Needs ReviewPublic

Authored by ekatz on Nov 11 2019, 12:14 PM.

Details

Summary

Calculating SCEVs can be cumbersome, and may take very long time (even hours, for very long expressions). To prevent recalculating expressions over and over again, we cache them.
This change add cache queries to key positions, to prevent recalculation of the expressions.

Fix PR43571.

Diff Detail

Event Timeline

ekatz created this revision.Nov 11 2019, 12:14 PM

compile time numbers? how this speeds up eg. bootstrap of clang?

ekatz added a comment.Nov 12 2019, 3:13 AM

compile time numbers? how this speeds up eg. bootstrap of clang?

It seems like a simple performance boost, but this actually fixes PR43571, and I quote, "...it's been nearly 10hrs".
With this change it takes a less than a second.

fhahn added a subscriber: fhahn.Tue, Nov 12, 2:02 PM

Can you explain where the claimed speed-up is coming from? We were missing cases in the caching before? Is it simply not rebuilding the folding set node multiple times? Something else?

This change appears to have a mix of functional change, and refactoring intermixed. It would make review much easier if you were to split them apart. In particular, a couple of suggestions:

  1. The getOrCreateX functions appear to be easily implementable with the new infrastructure. Please do so, as this avoids changes to the (delicate!) flag handling which will otherwise slow review.
  2. Separate any removal/movement of caching routine calls into a separate change if possible. (Not sure this is possible, which is the source of confusion.)
llvm/include/llvm/Analysis/ScalarEvolution.h
1873 ↗(On Diff #228750)

Having the insert point as part of the key seems odd structurally. I'm also concerned about invalidation if the cache is otherwise modified between setting the IP and using it.

1880 ↗(On Diff #228750)

Looks like this forward declare can be in the cpp.

llvm/lib/Analysis/ScalarEvolution.cpp
1274

It seems natural this should be before the rewrite rules?

ekatz updated this revision to Diff 230815.Sun, Nov 24, 11:07 AM
ekatz retitled this revision from [SCEV] Optimize SCEV cache usage to [SCEV] Add missing cache queries.
ekatz edited the summary of this revision. (Show Details)

Remove unrelated changes. Now only the missing cache queries are present in the patch.