<div>Hi Andy,</div><div><br></div><div>Editorializing somewhat, I think it's unfortunate that people expect constexpr evaluation to be asymptotically more efficient than the runtime evaluation of the same functions, and I'm a little concerned that this caching will lead people to write code which has surprising poor performance in the cases where a constexpr function gets invoked dynamically. But... g++ already implements a similar optimization, so we'll probably need this to handle code written for it.<br>
</div><div><br></div>Have you measured the impact of this cache on "well-written" constexpr functions (those not requiring memoization for efficiency)? What's the memory usage and performance impact on, say, test/SemaCXX/constexpr-turing.cpp, with a more complex Turing machine? Can you provide some figures for typical and worst-case slowdowns?<div>
<br></div><div>There is a nasty corner case where the value of a global changes between successive calls to a constexpr function (this happens if a variable is read after it's zero-initialized but before it's dynamically initialized, from a constexpr function called within its own initializer). Your patch doesn't seem to handle that (though perhaps I just missed it).</div>
<div><br></div><div>Have you considered using llvm::FoldingSet for the cache? That would be particularly useful if we ever want to allow arbitrary literal types as non-type template arguments, or other such cases which might require reducing an APValue to a FoldingSetNodeID.</div>
<div><br></div><div>There seems to be a fair amount of APValue copying going on in this change. For large APValues, that's not cheap. Perhaps you could allocate the cache entry up-front, and evaluate the function arguments and return value directly into its APValue slots, or swap them in?</div>
<div><br></div><div>Does this have an impact on the constexpr recursion limit? For instance, if f(500) recurses to depth 500, and g(500) recurses to depth 500 then calls f(500), will the result of calling g(500) depend on whether I've called f(500) already in this translation unit? That seems like a highly unfortunate property, but it's consistent with how template instantiations behave at least.</div>
<div><br></div><div>Thanks for looking into this!</div><div>Richard</div><div><br><div class="gmail_quote">On Wed, Aug 22, 2012 at 9:29 AM, Andy Gibbs <span dir="ltr"><<a href="mailto:andyg1001@hotmail.co.uk" target="_blank">andyg1001@hotmail.co.uk</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
Attached here is the fourth of five patches for PR12850.<br>
<br>
This patch makes some alterations to VerifyDiagnosticConsumer (it seems I<br>
cannot leave it alone!) which are there to enable test-cases to check on<br>
the performance of the cache.  An additional -verify directive, namely<br>
<br>
  verify-fncache-stats<br>
<br>
is supported which generates a diagnostic note to the user or test-case<br>
which details the number of cache hits, the number of cached entries and<br>
the memory consumption (approximate).  This can be matched then with an<br>
appropriate "expected-note" in a test-case.<br>
<br>
I don't know whether people think this is a good way to achieve this,<br>
but I was looking for a way to particularly have the hits/count as part<br>
of test-cases to ensure no unintended performance regressions, and this<br>
seemed the best approach.<br>
<br>
I would welcome alternatives if this is not the way to do it.<br>
<br>
Cheers<br>
<span class="HOEnZb"><font color="#888888">Andy<br>
<br>
</font></span><br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div>