<div dir="ltr">I didn't have a lot of time to dig into the patch, but an off by default flag to speed things up with a reduced the diagnostic experience doesn't seem that valuable.<div><br></div><div>I thought there was a macro expansion note backtrace limit. Is there a way that we can leverage that to generate unique expansion source locations when the expansion level is low but cut them off when the expansion level is deep? I'm not a preprocessor expert, so can you elaborate on exactly what you're caching and reusing here?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 15, 2016 at 1:17 PM, Андрей Серебро <span dir="ltr"><<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div>Yep, of course it passes, since without using the new flag it's pretty much the same release-38. </div><div>I don't know, probably, either some new tests should be generated and clang with new flag should be run on them or we can just use old ones but somehow make clang run with new flag... </div><div> </div><div>15.06.2016, 21:28, "Bruno Cardoso Lopes" <<a href="mailto:bruno.cardoso@gmail.com" target="_blank">bruno.cardoso@gmail.com</a>>:</div><div class="HOEnZb"><div class="h5"><blockquote type="cite"><p>Hi, the speedups look great. Do this patch passes a clean `ninja check-clang`?<br><br>On Wed, Jun 15, 2016 at 8:30 AM, Андрей Серебро <<a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a>> wrote:</p><blockquote> Ouch, forgot to attach stats. Here they are.<br><br> 15.06.2016, 18:28, "Андрей Серебро" <<a href="mailto:andy.silv@yandex.ru" target="_blank">andy.silv@yandex.ru</a>>:<br><br> So, taking your advice I got rid of caching for now and concentrated on<br> turning off source locations.<br><br> I have introduced new flag -no-macro-exploc-tracking. Passing it to compiler<br> does exactly what is intended to do - we don't generate expansion locations<br> for macro tokens. To evaluate correct __LINE__ expansion, we just memorize<br> the top most macro that is currently being expanded (it is ok since all<br> macro expansions turn to produce one-liners... but there are some cases with<br> multiline arguments for function-like macros, I didn't really get right now<br> should we do something with it).<br><br> I have tested it tough on boost 1.61. Among first 11000 files at least the<br> success of compilation is the same for patched and clean release 38.<br><br> Average boosting of preprocessing is 1.2 times. Also cases in which clang<br> ran out of source locations will be less frequent I hope. At least, here is<br> a fine example where compiler doesn't fall with the flag and fall without:<br><br> clang -c -ftemplate-depth-32 -O3 -finline-functions -Wno-inline -Wall<br> -pedantic -Wno-variadic-macros -pedantic-errors -std=c++11 -I<br> $BOOST_1_61_HOME -no-macro-exploc-tracking<br> $BOOST_1_61_HOME/libs/vmd/test/test_doc_modifiers_return_type.cpp<br><br> If I don't put the flag, the process hangs (at least, on my machine).<br><br> I attach the patch, hope for some feedback.<br><br> P.S. Also here are preprocessing benchmarks for boost.<br><br> 09.06.2016, 16:19, "Андрей Серебро" <<a href="mailto:andy.silv@yandex.ru" target="_blank">andy.silv@yandex.ru</a>>:<br><br> Interesting moment about Andy's comment. I found out that GCC already has<br> (if I understood correctly) teh feature with expansion tracking depth.<br><br> The option is -ftrack-macro-expansion[=level] and it's description can be<br> found here:<br> <a href="https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html" target="_blank">https://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html</a><br><br> As I understand, clang doesn't bother with this yet.<br><br> 05.06.2016, 01:38, "Андрей Серебро" <<a href="mailto:andy.silv@yandex.ru" target="_blank">andy.silv@yandex.ru</a>>:<br><br> Well, you know, this splitting sounds like a good idea. I guess I'll try to<br> follow your advice, for it's really somehow strange to solve two tasks in a<br> time.<br><br> I weighed the advantages and disadvantages and I guess that saving source<br> locations would just vanish any sense from the activity of boosting macro<br> expansion, because we will need to do all the same works as we did without<br> caching at all: to create proper locations we will need to go down from the<br> very top macro downside for every token. So, the idea from my previous post<br> was not that sane - saving proper source locations means just give up the<br> idea at all, it will barely reduce time consumption at all.<br><br> You are right about debugging purpose of SL, and I think the idea with<br> limitation on depths, for which info will be present, is solid. Looks like<br> more flexible then complete turning off.<br><br> About scratch space - to be honest, I haven't thought about it yet. I didn't<br> even look in this side, so that's good you have pointed it. First of all, I<br> need to understand what is this scratch space all about. =)<br><br> Such things like pragmas inside macro definitions which spoil caching can be<br> overcome by just forbidding caching in certain situations, yet of course all<br> such situations should be defined carefully.<br><br> 04.06.2016, 10:43, "Andy Gibbs" <<a href="mailto:andyg1001@hotmail.co.uk" target="_blank">andyg1001@hotmail.co.uk</a>>:<br><br> On Friday 03 Jun 2016 14:33:37 Андрей Серебро wrote:<br><br>  Ok, that would be a miracle.<br><br>  As I understand, unfortunately getting rid of proper source locations<br> causes<br>  big problems, yet it's currently hard for me to find some small samples on<br>  which everything crashes. I mean, it would be great to reduce the crash<br>  code dramatically, for now the examples of misbehavior are huge.<br>  I think a good understanding of interactions between SourceManager, Lexer<br>  and Parser is needed now because it's somewhere in between their<br>  communication, so to say, which turns to be broken after the patch. I see a<br>  possibility in trying to save proper SourceLocations, yet I'm not sure it<br>  will be such an easy feature.<br>  I wonder if I can find some person who deeply understands the concept of<br>  SourceLocations so that it's possible to discuss the underlying problems?<br>  Guys, I need your piece of advice :|<br><br><br> My suggestion, but others should jump in here too, is that you split the<br> patch<br> up into two separate aims.<br><br> The first aim would be the reduction of SourceLocation information.<br><br> The second aim would be caching of previous macro expansions (from what I<br> understand of your patch, I think this is what you are attempting?).<br><br> I think this would be a benefit since it may be easier to get the patches<br> working if you work on the two parts independently. Plus you indicated that<br> the first still has a sizeable benefit. And I think the second will give you<br> problems (detailed below).<br><br> I'm afraid I am not able to help you much on the interactions of the<br> SourceManager, Lexer and Parser. There are others on this mailing list who<br> would be able to help you much better in this regard.<br><br> Regarding reduction of SourceLocation information, these are my thoughts.<br> Please note that I haven't assessed the practicality of this, and I'm<br> writing<br> as I think so it may not even be coherent! Caveats aside, you talked<br> previously of having this as a compiler switch. I think this is a good idea<br> since the SourceLocation information is needed for debugging macro<br> expansions.<br> I would suggest implementing this switch as a depth counter so that<br> SourceLocation information is only once a macro expansion reaches a certain<br> depth the SourceLocation information will be elided. In a sense, the<br> important information of a macro expansion is the SourceLocations of the<br> original tokens going into the macro and the SourceLocation of the final<br> scratch buffer. At this point, I'm wondering whether it is useful to have<br> SourceLocation information for concatenated tokens... Maybe someone else can<br> give there thoughts here.<br><br> The other thing you can think about here is that of actually releasing a<br> scratch buffer no longer in use. I think clang doesn't do this currently<br> (probably because of keeping all the SourceLocation information), but if you<br> looked at having a reference count on a scratch buffer then maybe it will be<br> possible to release it once its last use is complete. This has the benefit<br> of<br> also freeing memory... and may also improve the speed of the compiler.<br> Again,<br> others should give their thoughts here. Personally, I would be very<br> interested to see if this was a possible line of improvement since with<br> extensive macro expansions, the memory consumption of clang is huge.<br><br> In fact, thinking about this, this may be the first place to start -- it is<br> sort of linked to eliding SourceLocation information, but it has the<br> advantage<br> that you are freeing the whole scratch buffer since it has been incorporated<br> into another higher-level scratch buffer, and so is no longer referenced<br> anywhere and its disappearance shouldn't (hopefully!) cause so many problems<br> in clang diagnostics, for example, which even so would need to be adapted to<br> skip these elided macro expansion steps.<br><br> Finally, regarding the caching of previous macro expansions, you need to be<br> very careful because macro definitions can change, yes even inside a macro<br> expansion too (think pragma pop_macro). For that matter, you need to think<br> about how to handle any _Pragma expansion since these can cause side<br> effects.<br> This is why I would leave this side of your patch until last. It will be the<br> hardest to test and guarantee correctness.<br><br> Keep up the good work! :o)<br><br> Cheers,<br> Andy<br><br><br><br> --<br> Regards,<br> Andrei Serebro<br> tel. <span><a href="tel:%2B79111758381" value="+79111758381" target="_blank">+79111758381</a></span><br><br><br><br><br> --<br> Regards,<br> Andrei Serebro<br> tel. <span><a href="tel:%2B79111758381" value="+79111758381" target="_blank">+79111758381</a></span><br><br><br><br><br> --<br> Regards,<br> Andrei Serebro<br> tel. <span><a href="tel:%2B79111758381" value="+79111758381" target="_blank">+79111758381</a></span><br><br><br><br><br> --<br> Regards,<br> Andrei Serebro<br> tel. <span><a href="tel:%2B79111758381" value="+79111758381" target="_blank">+79111758381</a></span><br><br><br> _______________________________________________<br> cfe-dev mailing list<br> <a href="mailto:cfe-dev@lists.llvm.org" target="_blank">cfe-dev@lists.llvm.org</a><br> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br><br></blockquote><p><br><br><br></p><span>-- <br>Bruno Cardoso Lopes<br><a href="http://www.brunocardoso.cc/" target="_blank">http://www.brunocardoso.cc</a><br></span></blockquote><div> </div><div> </div><div>-- </div><div>Regards,<br> Andrei Serebro</div><div>tel. <a href="tel:%2B79111758381" value="+79111758381" target="_blank">+79111758381</a></div><div> </div></div></div><br>_______________________________________________<br>
cfe-dev mailing list<br>
<a href="mailto:cfe-dev@lists.llvm.org">cfe-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev</a><br>
<br></blockquote></div><br></div>