[PATCH] D34273: [SCEV] Use depth limit instead of local cache for SExt and ZExt

[Maxim Kazantsev via llvm-commits]

Hi Daniel,

I believe that using cache here was a wrong idea initially. It was done in assumption that we meet the same values throughout recursions, which was true for the particular test when the patch with was developed, but actually there is no reason why it should be true in general. If you look into the test of PR33431, its compilation takes few minutes either with or without cache. Cache is just useless there, and as side effect it consumes 800M of memory.

So the root problem is that the recursion here can be deep, leading to long compilation or stack overflows, and massive memory consumption by cache is one of side effects of that. My intention was to deal with the root problem, not make a workaround for workaround.

-- Max

From: Daniel Berlin [mailto:dberlin at dberlin.org]
Sent: Thursday, June 22, 2017 8:52 AM
To: reviews+D34273+public+1b835f147de217a1 at reviews.llvm.org; Max Kazantsev via Phabricator <reviews at reviews.llvm.org>
Cc: Maxim Kazantsev <max.kazantsev at azul.com>; Wei Mi <wmi at google.com>; Sanjoy Das <sanjoy at playingwithpointers.com>; Philip Reames <listmail at philipreames.com>; Junbum Lim <junbuml at codeaurora.org>; llvm-commits <llvm-commits at lists.llvm.org>
Subject: Re: [PATCH] D34273: [SCEV] Use depth limit instead of local cache for SExt and ZExt

To control the size of the cache, why not just LRU it instead?
Limiting recursion depth seems like a really big hammer to use here.
Is there really no better way?



On Fri, Jun 16, 2017 at 5:02 AM, Max Kazantsev via Phabricator via llvm-commits <llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
mkazantsev created this revision.
Herald added a subscriber: mzolotukhin.

In https://reviews.llvm.org/rL300494 there was an attempt to deal with excessive compile time on
invocations of getSign/ZeroExtExpr using local caching. This approach only
helps if we request the same SCEV multiple times throughout recursion. But
in the bug PR33431 we see a case where we request different values all the time,
so caching does not help and the size of the cache grows enormously.

In this patch we remove the local cache for this methods and add the recursion
depth limit instead, as we do for arithmetics. This gives us a guarantee that the
invocation sequence is limited and reasonably short.


https://reviews.llvm.org/D34273

Files:
  include/llvm/Analysis/ScalarEvolution.h
  lib/Analysis/ScalarEvolution.cpp
  lib/Transforms/Utils/SimplifyIndVar.cpp
  test/Analysis/ScalarEvolution/limit-depth.ll


_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170622/fd27edee/attachment.html>