mboehme added a comment. In D128097#3604295 <https://reviews.llvm.org/D128097#3604295>, @yurai007 wrote:
> Disclaimer: I'm not front-end guy. > > After running 7zip benchmark under perf and comparing origin (7acc88be031 > <https://reviews.llvm.org/rG7acc88be0312c721bc082ed9934e381d297f4707>) with > fix (0d300da799b0 > <https://reviews.llvm.org/rG0d300da799b06931eb6b974198d683548a8c8392>) > following diff can be seen: Thanks a lot for helping me out here! > Instructions change | Symbol > +0.12% | clang::Preprocessor::getMacroDefinition Hm, this surprises me. AFAICT, all of the relevant places that this gets called from are in the lexer… and I don’t think my patch should have affected what the parser does with the lexer. I almost suspect that this regression is due to some other change that happened between 7acc88be031 <https://reviews.llvm.org/rG7acc88be0312c721bc082ed9934e381d297f4707> and 0d300da799b0 <https://reviews.llvm.org/rG0d300da799b06931eb6b974198d683548a8c8392>? > +0.07% | GetDeclSpecTypeForDeclarator I think you may potentially have found the reason for this – see below. > +0.04% | clang::Parser::ParseDirectDeclarator > +0.03% | clang::Sema::ActOnFunctionDeclarator > +0.03% | clang::Sema::ProcessDeclAttributes An increase here is expected as my patch did add new code to `ProcessDeclAttributes()` to handle the “sliding” logic; as such, this increase in time seems hard to avoid. > In 8c7b64b5ae2a > <https://reviews.llvm.org/rG8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb> commit > those 2 code changes catch my eyes as (maybe) relevant to above perf diff: > > 1. Change in Parser::ParseDeclGroup. That function looks quite heavy and it's > caller of ParseDirectDeclarator/ActOnFunctionDeclarator (and even > getMacroDefinition?) Here’s what changed in that function: - ParsingDeclarator D(*this, DS, Context); + // Consume all of the attributes from `Attrs` by moving them to our own local + // list. This ensures that we will not attempt to interpret them as statement + // attributes higher up the callchain. + ParsedAttributes LocalAttrs(AttrFactory); + LocalAttrs.takeAllFrom(Attrs); + ParsingDeclarator D(*this, DS, LocalAttrs, Context); (`Attrs` is a new parameter of `ParseDeclGroup()`.) I’m not sure that this change per se had a significant impact on compile time though, as `ParseDeclGroup()` itself doesn’t show up in the diff above. The change also shouldn’t really affect how much time we spend in `ParseDirectDeclarator()` or `ActOnFunctionDeclarator()`. I’m inclined to say that the extra time spent in `ParseDirectDeclarator()` and `ActOnFunctionDeclarator()` is due to some other change that happened in the meantime, even though I realize that this sounds like a cheap excuse… > 2. Change in GetDeclSpecTypeForDeclarator. After 8c7b64b5ae2a > <https://reviews.llvm.org/rG8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb> > distributeTypeAttrsFromDeclarator is called unconditionally which maybe > matters. Thanks for pointing this out to me – this could be the explanation for the 0.07% slowdown that you saw in `GetDeclSpecTypeForDeclarator()`, as this is the only change in that function. Of course, the other possibility is that GetDeclSpecTypeForDeclarator() is now getting called more often, but I don’t immediately see why that should be the case. I had removed the if statement because it’s superfluous from a behavioral point of view, but it may well be important for performance. I’ve prepared https://reviews.llvm.org/D128439 to re-add that statement. I unfortunately haven’t yet been able to get the test suite running. Since you do have a running setup, do you think you could test the compile time impact of that patch on 7zip? Please do this only if it’s not a big deal – otherwise, I’ll just submit the patch for review, as it seems that it should have at least a small positive influence on performance. Again, thanks for your input here! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128097/new/ https://reviews.llvm.org/D128097 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
