[PATCH] D54547: PTH-- Remove feature entirely-

[James Y Knight via Phabricator via cfe-commits]

jyknight accepted this revision.
jyknight added a comment.
This revision is now accepted and ready to land.

Since you've already submitted a fix to Boost, 
https://github.com/boostorg/build/commit/3385fe2aa699a45e722a1013658f824b6a7c761f,
 I think this is fine to remove.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54547/new/

https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added inline comments.



Comment at: include/clang/Lex/Preprocessor.h:396
+  /// This is an alias for CurLexer.
   PreprocessorLexer *CurPPLexer = nullptr;
 

erichkeane wrote:
> riccibruno wrote:
> > Would it make sense to remove this alias now that it
> > will always refer to `CurLexer` ?
> There seems to be some logic around this that I'm not totally sure about 
> dealing with IsFileLexer.  
Sounds fine to me. Someone more familiar with this can
always come back later and rework this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54547/new/

https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 2 inline comments as done.
erichkeane added inline comments.



Comment at: include/clang/Lex/Preprocessor.h:396
+  /// This is an alias for CurLexer.
   PreprocessorLexer *CurPPLexer = nullptr;
 

riccibruno wrote:
> Would it make sense to remove this alias now that it
> will always refer to `CurLexer` ?
There seems to be some logic around this that I'm not totally sure about 
dealing with IsFileLexer.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54547/new/

https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

I looked at how `ASTReader` create the `IdentifierInfo`s returned by 
`IdentifierInfo *ASTReader::get(StringRef Name)`,
and I ended up in `ASTIdentifierLookupTrait::ReadData`, which calls among other 
things `IdentifierTable::getOwn`.
The `IdentifierInfo` created by `getOwn` will always have the `Entry` pointer 
set and therefore I think it is
possible to remove the `if (Entry)` in `IdentifierInfo::getLength` and 
`IdentifierInfo::getNameStart`.

I made a patch implementing this change here D54866 
.




Comment at: include/clang/Lex/Preprocessor.h:396
+  /// This is an alias for CurLexer.
   PreprocessorLexer *CurPPLexer = nullptr;
 

Would it make sense to remove this alias now that it
will always refer to `CurLexer` ?



Comment at: unittests/Tooling/DiagnosticsYamlTest.cpp:85
 "FilePath:'path/to/source2.cpp'\n"
-"Replacements:\n"
+"Replacements:[]\n"
 "...\n",

It was fixed in `r346884 : [Support] Teach YAMLIO about polymorphic types`
so you can remove this from your patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54547/new/

https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Vladimir Voskresensky via Phabricator via cfe-commits]

voskresensky.vladimir added a comment.

In https://reviews.llvm.org/D54547#1301359, @erichkeane wrote:

> > I thought clang-d service is using it to speed up indexing.
>
> Presumably, I could also just make PTH use another bit or two for the TokenID 
> and it would work fine.  However, when I mentioned this on IRC the general 
> response was that it is a 'failed experiment'.  In your use case, I wonder 
> why you couldn't just use PCH and get even further performance improvements?


To be fair, I don't remember exactly. :-)
I remember we shared the same PTH for all C and C++ files and built some 
preprocessor-based features for incomplete code written in editor based on that.

Hmm... One more thing I had to fix was: lex in mode where we emit all 
preprocessor tokens there as well (like "#define MACRO A" => #, define, MACRO, 
A)

PHT was also used for rebuilding PCH, when something is changed. Because often 
single changed file can completely invalidate PCH, while PTH just contain 
pp-lexed tokens which doesn't carry semantic. So, PTHManager could skip 
providing tokens for changed file, but keep proved them for all other files.


https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

> I thought clang-d service is using it to speed up indexing.

I believe that clangd is using PCH and not PTH.
(by the highly sophisticated method of grepping for pth and pch inside clangd/)


https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In https://reviews.llvm.org/D54547#1301351, @voskresensky.vladimir wrote:

> I have some experience with PTH implementation, because had to fix it for 
> Java-port of Clang (https://github.com/java-port/clank).
>
> It was sometime ago, but making it completely workable was not hard.
>  As I remember the key fix was just to have PTH be EMITTED using raw-Lex mode 
> (where real keyword-IDs are not used, so all Token Kinds nicely fit 8-bits).
>  It worked, because on token automatically became keyword by PTHLexer::Lex:
>  ...
>
>  // Change the kind of this identifier to the appropriate token kind, e.g.
>  // turning "for" into a keyword.
>   Tok.setKind(II->getTokenID());
>
> ...
>
> We used PTH, because multiple translation units are parsed in the context of 
> single run.
>  In this case preprocessing phase was upto 10x faster when token stream was 
> deserialized from PTH (i.e. for all system headers lexed once).
>  Also it was helpful, because we were able to parse from concurrent threads 
> using the same shared immutable mmaped PTH.
>
> I thought clang-d service is using it to speed up indexing.


Presumably, I could also just make PTH use another bit or two for the TokenID 
and it would work fine.  However, when I mentioned this on IRC the general 
response was that it is a 'failed experiment'.  In your use case, I wonder why 
you couldn't just use PCH and get even further performance improvements?


https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Vladimir Voskresensky via Phabricator via cfe-commits]

voskresensky.vladimir added a comment.

I have some experience with PTH implementation, because had to fix it for 
Java-port of Clang (https://github.com/java-port/clank).

It was sometime ago, but making it completely workable was not hard.
As I remember the key fix was just to have PTH be EMITTED using raw-Lex mode 
(where real keyword-IDs are not used, so all Token Kinds nicely fit 8-bits).
It worked, because on token automatically became keyword by PTHLexer::Lex:
...

 // Change the kind of this identifier to the appropriate token kind, e.g.
 // turning "for" into a keyword.
  Tok.setKind(II->getTokenID());

...

We used PTH, because multiple translation units are parsed in the context of 
single run.
In this case preprocessing phase was be upto 10x faster when take token stream 
was deserialized from PTH (i.e. for all system headers lexed once).
Also it was also helpful, because we were able to parse from concurrent thread 
using the same shared immutable PTH.

I thought clang-d service is using it to speed up indexing.


https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In https://reviews.llvm.org/D54547#1301253, @arphaman wrote:

> Would it be better to deprecate the use of PTH right now for the current 
> release, so that the users will be aware that we will remove PTH support in 
> LLVM 9 once they upgrade to LLVM 8? Then we can remove it right after LLVM 8 
> branch is created.


I considered that.  However, it is:
A- Really broken
B- Only has 1 user
C- Only accessible by a cc1 option, users aren't supposed to use these anyway.

Because of this, I/we seem to have little sympathy for the users.


https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Alex Lorenz via Phabricator via cfe-commits]

arphaman added a comment.

Would it be better to deprecate the use of PTH right now for the current 
release, so that the users will be aware that we will remove PTH support in 
LLVM 9 once they upgrade to LLVM 8? Then we can remove it right after LLVM 8 
branch is created.


https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

In https://reviews.llvm.org/D54547#1299883, @erichkeane wrote:

> Added to the release notes.  Also, an email was sent out to cfe-dev.
>
> @riccibruno and I are separately looking into IdentifierInfo, because it 
> seems that there are some pretty massive optimization opportunities if we can 
> remove PTH/indirect identifiers.


As discussed on IRC:
In any case this is not blocking the removal of PTH.  The code around 
`IdentifierInfo`
is very performance sensitive and it will take some time to evaluate the 
possible
changes, if any.


https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Bruno Ricci via Phabricator via cfe-commits]

riccibruno added a comment.

I don't think I can comment more broadly on this, but
there is some PTH-related code in `Basic/IdentifierTable`,
in `IdentifierInfo::getNameStart` and `IdentifierInfo::getLength`.
Maybe this should be removed too ?


Repository:
  rC Clang

https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D54547#1299105, @chandlerc wrote:

> Should likely add release notes about this.
>
> Also might be worth sending a note to cfe-dev as a heads up and give folks 
> some time to say "wait wait".


+1 to both of these points, but if doesn't cause too big of a burden for folks, 
I'm all in favor of it.


Repository:
  rC Clang

https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Chandler Carruth via Phabricator via cfe-commits]

chandlerc added a comment.

Should likely add release notes about this.

Also might be worth sending a note to cfe-dev as a heads up and give folks some 
time to say "wait wait".


Repository:
  rC Clang

https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54547: PTH-- Remove feature entirely-

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.



Comment at: include/clang/Driver/Options.td:261
   MetaVarName<"">;
-def ccc_pch_is_pch : Flag<["-"], "ccc-pch-is-pch">, InternalDriverOpt,
-  HelpText<"Use lazy PCH for precompiled headers">;

The default behavior is exactly to use this flag.  I removed the flag since 
googling showed that it is never really used.



Comment at: lib/Frontend/CompilerInstance.cpp:384
+  getSourceManager(), getPCMCache(), *HeaderInfo, *this,
+  /*IdentifierInfoLookup=*/nullptr,
   /*OwnsHeaderSearch=*/true, TUKind);

IdentifierInfoLookup is an abstraction around ASTReader and (previously) 
PTHManager.  It seems that this abstraction could still be useful, so I've 
chosen to leave it in place.

When PTH wasn't enabled, PTHMgr was nullptr anyway, so this just uses that 
instead.



Comment at: unittests/Tooling/DiagnosticsYamlTest.cpp:85
 "FilePath:'path/to/source2.cpp'\n"
-"Replacements:\n"
+"Replacements:[]\n"
 "...\n",

I'm unsure about what caused this change.  It seems to me that these two should 
be equivalent, and hopefully someone will correct me if I'm wrong.


Repository:
  rC Clang

https://reviews.llvm.org/D54547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits