This is an archive of the discontinued LLVM Phabricator instance.

[HLSL] Support PCH for cc1 mode
ClosedPublic

Authored by python3kgae on Aug 22 2022, 5:01 PM.

Details

Summary

Add HLSLExternalSemaSource as ExternalSemaSource instead of ASTContext::ExternalSource when PCH is included.

This allows a different external source to be set for the AST context.

Diff Detail

Event Timeline

python3kgae created this revision.Aug 22 2022, 5:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 5:01 PM
Herald added a subscriber: Anastasia. · View Herald Transcript
python3kgae requested review of this revision.Aug 22 2022, 5:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2022, 5:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
beanz added a comment.Aug 22 2022, 6:12 PM

This change doesn't look like it is NFC to me. If I'm understanding correctly it results in the HLSLExternalSemaSource being set for Sema, but allows a different external source to be set for the AST context. That doesn't seem unreasonable to me, but perhaps we should have a test case that verifies that it works correctly?

Perhaps a test case with a PCH that uses something populated by the HLSL external source? That would demonstrate that the AST source for the PCH works and that it doesn't interfere with the HLSL source.

When there's no external source set, add HLSLSema as ExternalSource so ASTWriter can access HLSLSema.

Add newline at end of file

python3kgae retitled this revision from [NFC] [HLSL] Add HLSLExternalSemaSource as ExternalSemaSource instead of ASTContext::ExternalSource. to [HLSL] Support PCH for cc1 mode.Aug 23 2022, 10:29 AM
python3kgae edited the summary of this revision. (Show Details)
beanz added inline comments.Aug 23 2022, 10:41 AM
clang/test/SemaHLSL/pch.hlsl
5 ↗(On Diff #454889)

I want to make sure I'm following this test.

You compile this file to a PCH (%t), then compile /dev/null using this %t as the PCH.

IIUC, that doesn't actually verify that PCH and the HLSL sema work together. There is no test here that pulls from the PCH and HLSL Sema in the same translation unit.

My suggestion would be to:
(1) move this test under AST/HLSL (since it is an AST test, not a Sema test)
(2) put a header under AST/HLSL/Inputs that (like this file) contains a function that uses HLSL types
(3) make the test file separate from the header, and have it use both the function in the header and some completely different HLSL-provided type (like declare a RWBuffer).
(4) your test can then generate the PCH, and use the PCH to compile the source verifying that both AST sources work

Reuse decls already in PCH when initailize HLSLExternalSemaSource.

Fix build error

Fix format.

beanz added inline comments.Aug 30 2022, 10:12 AM
clang/include/clang/Sema/HLSLExternalSemaSource.h
57

IIUC, this code just exists to make sure that the ASTReader deserializes before the external sema source starts adding things. Is that correct?

If so, you don't need to do this, instead you can just add this code to InitializeSema() to force the ASTReader to de-serialize the decls:

// If the translation unit has external storage force external decls to load.
if (AST.getTranslationUnitDecl()->hasExternalLexicalStorage())
    (void)AST.getTranslationUnitDecl()->decls_begin();
clang/lib/Sema/HLSLExternalSemaSource.cpp
399

I think the core of what you'e doing here is not far off, but I think this pattern doesn't scale or cover all the use cases that matter.

The cases that come to my mind are:

  1. PCH has a forward declaration, source doesn't use the type so we shouldn't complete the type.
  2. PCH has a forward declaration, source uses the type, so we need to generate a complete decl.
  3. PCH has a complete declaration, so we should re-use the declaration and not need to do anything.

We also need to keep in mind that we're going to keep adding more and more types, so this pattern where each new type needs to add code to lookup is going to be a challenge. It is also going to be a challenge to manage the complexity of PCH defines one thing but not another.

One thing you should look at is how clang handles the following C++ code:

template<typename T>
class Foo;

template<typename T>
class Foo {
  T Val;
};

This results in two decls. One for the forward decl and the second for the definition. The definition lists the forward declaration as the previous decl. We should do that here.

When we generate the HLSL namespace, we can do a lookup and if we find a previous decl, set the previous decl.

When we generate any of the builtin types, we can lookup the previous decl, and annotate as the previous decl. We'll can also handle the case where a decl from the PCH is complete. If the PCH provides a complete decl, we can skip generating any decls for it, and the lookups should just work.

python3kgae added inline comments.Aug 31 2022, 12:14 PM
clang/lib/Sema/HLSLExternalSemaSource.cpp
399

Set previous decl not work.
It goes to "For most kinds of declaration, it doesn't really matter which one we pick." before checking PreviousDecl.

   // For most kinds of declaration, it doesn't really matter which one we pick.
  if (!isa<FunctionDecl>(DUnderlying) && !isa<VarDecl>(DUnderlying)) {
    // If the existing declaration is hidden, prefer the new one. Otherwise,
    // keep what we've got.
    return !S.isVisible(Existing);
  }



  // Pick the newer declaration; it might have a more precise type.
  for (Decl *Prev = DUnderlying->getPreviousDecl(); Prev;
       Prev = Prev->getPreviousDecl())
    if (Prev == EUnderlying)
      return true;
  return false;

Move reuse decl in PCH into BuiltinTypeDeclBuilder.

python3kgae marked an inline comment as done.Aug 31 2022, 1:50 PM
python3kgae added inline comments.
clang/include/clang/Sema/HLSLExternalSemaSource.h
57

Still need this to make sure HLSLSema and ExternalSema are initialized before MultiplexExternalSemaSource.

beanz added inline comments.Aug 31 2022, 2:44 PM
clang/include/clang/Sema/HLSLExternalSemaSource.h
57

In FrontendAction, where you are creating the Chained source, you can instead create a Multiplex source, and put the PCH external source in as the first source and the HLSL one second. The PCH will get initialized first, the HLSL one can force the PCH one to populate the decls.

clang/lib/Sema/HLSLExternalSemaSource.cpp
397

Rather than reusing the namespace you can set the PCH one as the previous decl for the one you're declaring here. That will result in them basically being merged because Namespaces can have multiple decls.

The advantage to this is that the PCH namespace will be marked in the AST as a deserialized decl but the HLSL one won't. It will make debugging and AST visualization easier.

479

If the decl you put in here is the canonical decl, and you do the lookup by canonical decl. The conflicting decls shouldn't be an issue. That does require that you mark the previous decl so that they are known to be the same declaration.

python3kgae marked an inline comment as done.Aug 31 2022, 9:52 PM
python3kgae added inline comments.
clang/include/clang/Sema/HLSLExternalSemaSource.h
57

Asked how to resolve the issue in https://discourse.llvm.org/t/is-it-possible-for-multiplexexternalsemasource-to-own-the-sources/64990

Hope someone can explain why multiplexexternalsemasource doesn't own the Sources.

Rebase to use MultiplexExternalSemaSource.

python3kgae added inline comments.Sep 2 2022, 1:12 PM
clang/lib/Sema/HLSLExternalSemaSource.cpp
62

If comment this line out. The test will pass.

Merge fix from Chris.

Only add HLSLSema when not has it in ExternalSource.

Only add HLSLExternalSemaSource when ExternalSource is not HLSLExternalSemaSource.

beanz added inline comments.Sep 9 2022, 11:02 AM
clang/lib/Frontend/FrontendAction.cpp
1036

This seems off. The multi source should always have at least two external sema sources.

Is there any situation where the HLSL sema would already be added?

beanz added a subscriber: aaron.ballman.

Looping @aaron.ballman in here too. I think we need some eyes from outside our team on this before landing it.

Remove useless code since ExternalSource is not be reused when there're more than one Input files.

python3kgae marked an inline comment as done.Sep 9 2022, 11:45 AM
python3kgae added inline comments.
clang/lib/Frontend/FrontendAction.cpp
1036

Remove. I thought BeginSourceFile would reuse the same ExternalSource.

python3kgae marked an inline comment as done.

Fix test fail caused by not add HLSL builtin Resource type.

aaron.ballman added inline comments.Sep 14 2022, 10:32 AM
clang/lib/Sema/HLSLExternalSemaSource.cpp
50

99.9% sure this is safe because we're looking up a tag name and I can't think of any way that might return multiple results for the same identifier, but bringing it up just in case anyone else knows of some obscure extension where that's possible.

55

Is it possible that this finds a different kind of tag, like an enumeration?

85–86

A downside to this pattern is that we need to repeat the "if it's a complete definition, don't do anything" predicate in basically any mutating member function. Getting that wrong will lead to hard-to-debug issues with PCH, as I understand it. But I don't think there's a cleaner way to do that without doing far more invasive changes.

378

How certain are you that there's only one result possible here?

387
459–461

We made a lookup result but then do nothing with it?

python3kgae marked 4 inline comments as done.

Code cleanup per comments.

clang/lib/Sema/HLSLExternalSemaSource.cpp
55

No, enum with the same name will get an error when building the PCH.

378

I cannot think of a case more than one result is here.

459–461

Removed.

beanz accepted this revision.Sep 20 2022, 12:58 PM

One small nit, otherwise looks good.

clang/include/clang/Sema/HLSLExternalSemaSource.h
18

nit: This seems to be unused

This revision is now accepted and ready to land.Sep 20 2022, 12:58 PM
python3kgae marked 2 inline comments as done.

Move #include "clang/Sema/MultiplexExternalSemaSource.h" into FrontendAction.cpp

This revision was automatically updated to reflect the committed changes.