[PATCH] D128097: [Clang] Fix compile time regression caused by D126061.

[Martin Böhme via Phabricator via cfe-commits]

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