[HLSL] Support PCH for cc1 mode

[python3kgae]

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.

[beanz]

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.

[python3kgae]

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

[python3kgae]

Add newline at end of file

[beanz]

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

[python3kgae]

Reuse decls already in PCH when initailize HLSLExternalSemaSource.

[python3kgae]

Fix build error

[python3kgae]

Fix format.

[beanz]

clang/include/clang/Sema/HLSLExternalSemaSource.h

58 ↗

(On Diff #456387)

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

297 ↗

(On Diff #456387)

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]

clang/lib/Sema/HLSLExternalSemaSource.cpp

297 ↗

(On Diff #456387)

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;

[python3kgae]

Move reuse decl in PCH into BuiltinTypeDeclBuilder.

[python3kgae]

clang/include/clang/Sema/HLSLExternalSemaSource.h

58 ↗

(On Diff #456387)

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

[beanz]

clang/include/clang/Sema/HLSLExternalSemaSource.h

58 ↗

(On Diff #456387)

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

314 ↗

(On Diff #457081)

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.

411 ↗

(On Diff #457081)

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]

clang/include/clang/Sema/HLSLExternalSemaSource.h

58 ↗

(On Diff #456387)

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.

[python3kgae]

Rebase to use MultiplexExternalSemaSource.

[python3kgae]

clang/lib/Sema/HLSLExternalSemaSource.cpp

78 ↗

(On Diff #457684)

If comment this line out. The test will pass.

[python3kgae]

Merge fix from Chris.

[python3kgae]

Only add HLSLSema when not has it in ExternalSource.

[python3kgae]

Only add HLSLExternalSemaSource when ExternalSource is not HLSLExternalSemaSource.

[beanz]

clang/lib/Frontend/FrontendAction.cpp

1028

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]

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

[python3kgae]

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

[python3kgae]

clang/lib/Frontend/FrontendAction.cpp

1028

Remove. I thought BeginSourceFile would reuse the same ExternalSource.

[python3kgae]

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

[aaron.ballman]

clang/lib/Sema/HLSLExternalSemaSource.cpp

50 ↗

(On Diff #459519)

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 ↗

(On Diff #459519)

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

85–86 ↗

(On Diff #459519)

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 ↗

(On Diff #459519)

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

387 ↗

(On Diff #459519)

459–461 ↗

(On Diff #459519)

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

[python3kgae]

Code cleanup per comments.

clang/lib/Sema/HLSLExternalSemaSource.cpp

55 ↗

(On Diff #459519)

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

378 ↗

(On Diff #459519)

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

459–461 ↗

(On Diff #459519)

Removed.

[python3kgae]

Rebase

[beanz]

One small nit, otherwise looks good.

clang/include/clang/Sema/HLSLExternalSemaSource.h

18 ↗

(On Diff #461131)

nit: This seems to be unused

[python3kgae]

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

clang/lib/Frontend/FrontendAction.cpp

 //===--- FrontendAction.cpp -----------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
IntrusiveRefCntPtr<ExternalASTSource>
                      CI.getFrontendOpts().OverrideRecordLayoutsFile));
     CI.getASTContext().setExternalSource(Override);
   }
 
   // Setup HLSL External Sema Source
   if (CI.getLangOpts().HLSL && CI.hasASTContext()) {
     IntrusiveRefCntPtr<ExternalASTSource> HLSLSema(
         new HLSLExternalSemaSource());
-    CI.getASTContext().setExternalSource(HLSLSema);
+    CI.setExternalSemaSource(HLSLSema);
   }
 
   FailureCleanup.release();
   return true;
 }
 
 llvm::Error FrontendAction::Execute() {
   CompilerInstance &CI = getCompilerInstance();
 
   if (CI.hasFrontendTimer()) {
     llvm::TimeRegion Timer(CI.getFrontendTimer());
     ExecuteAction();
   }