[cxx-interop] Support function templates. #33053

zoecarver · 2020-07-22T19:28:11Z

This patch adds rudimentary support for C++ template functions in swift. There's still a lot lacking but, that mostly has to do with the clang type converter which can be updated with incremental follow up patches.

Unfortunately, there's some ugliness and unconventional logic in this patch. First, we have to generate and define the specialization in the SILGen phase (instead of when we import the function) so we have access to the argument types and substitution map. Similarly, in IRGen, we can't get the function pointer while visiting the function_ref instruction because we also need the substitution map to find the correct specialization of the template (this also means there may be problems when the compiler can't find the corresponding function_ref, i.e. if the function was passed as an argument).

CodaFi

Let's try to shop the modeling here.

CodaFi · 2020-07-22T23:50:09Z

lib/IRGen/IRGenSIL.cpp

@@ -2393,6 +2402,31 @@ void IRGenSILFunction::visitTryApplyInst(swift::TryApplyInst *i) {
  visitFullApplySite(i);
 }

+static void setCXXFunctionTemplateAddr(IRGenSILFunction &IGF,


It seems like we need some notion of a "foreign specialization" so this code doesn't have to completely special-case the apply path.

CodaFi · 2020-07-22T23:56:34Z

lib/IRGen/IRGenSIL.cpp

+        Builder.CreateStore(result, resultArgPtr, IGM.getPointerAlignment());
+      }
+      // Now we're done.
+      emission.clear();


This seems to imply that you have unclaimed parameter values along this open-coded path.

CodaFi · 2020-07-23T00:01:32Z

include/swift/AST/ClangTypeConverter.h

@@ -78,9 +78,10 @@ class ClangTypeConverter :
  /// declaration introduced by this converter, and if so, return the original
  /// Swift declaration.
  Decl *getSwiftDeclForExportedClangDecl(const clang::Decl *decl) const;
+
+  clang::QualType convert(Type type);


If I'm not mistaken, this is an implementation detail and should not be exposed. @varungandhi-apple?

Yeah, I'm a bit wary of this. Could we instead have a method on ClangTypeConverter which handles translating a templated C++ function type to a Swift function type, given a substitution map? I would've understood if this method needed to be used in a lot of places, but it only seems to have two external clients in this patch.

+1 to not making ClangTypeConverter public.

@varungandhi-apple, did you mean to have a method that translates a templated C++ function type to a Swift function type on ClangModuleLoader? That sounds like a place that would keep ClangTypeConverter encapsulated and allow us to do what we need in this PR.

ASTContext::Implementation already has a ClangTypeConverter member so, I think it makes sense to put this helper function there.

Sorry I missed this comment somehow.

did you mean to have a method that translates a templated C++ function type to a Swift function type on ClangModuleLoader? That sounds like a place that would keep ClangTypeConverter encapsulated and allow us to do what we need in this PR.

No, the type conversion methods are all on ClangTypeConverter. As Zoe said, the ASTContext carries around a ClangTypeConverter so that can be used (it is already being used in a handful of places). ClangModuleLoader shouldn't have methods for converting types.

CodaFi · 2020-07-23T00:07:24Z

lib/IRGen/IRGenSIL.cpp

+  if (auto fn = site.getInitiallyReferencedFunction()) {
+    auto clangDecl = fn->getClangDecl();
+    if (clangDecl && isa<clang::FunctionTemplateDecl>(clangDecl)) {
+      setCXXFunctionTemplateAddr(


We ought to aim to do this much, much earlier in the process here. This should be done as part of the lowering of a dedicated SIL instruction.

varungandhi-apple · 2020-07-23T06:56:59Z

On the overall approach: I don't think this is what we want to do in the medium-long term... I need to fix merge conflicts in #29691 and land that (I've been busy with other things, going to resume work on that tomorrow). Once we have Clang function types available in SIL, then we should be able to compute the type mapping when creating the corresponding Swift function type (right now it is eager, but we probably want to make it lazier later on). For templated functions, this would be unsubstituted types, since we are importing the declaration. During SILGen, I think we should be able to use the substitution map to translate the unsubstituted types into fully substituted types. So SILGen shouldn't need to do translation itself (all translations happen at the importing/AST layer), it only does substitutions.

Does that make sense and/or sound reasonable?

This is probably a bit fuzzy, I'm not sure about the exact details.

zoecarver · 2020-07-23T18:44:01Z

During SILGen, I think we should be able to use the substitution map to translate the unsubstituted types into fully substituted types. So SILGen shouldn't need to do translation itself (all translations happen at the importing/AST layer), it only does substitutions.

IIUC the difference between what you're suggesting and this patch is the SILGen'ed function_ref would point to a specialized function instead of the function template? I can get on board with that. It would reduce IRGen complexity too.

varungandhi-apple · 2020-07-23T19:13:40Z

That's a good summary of it, yeah. The broader point is that instead of having this type translation logic be defined and used in several places throughout the codebase, it would be nice to have as narrow an API as reasonably possible and only use in specific spots and have things "just work" (famous last words?), reducing coordination between different, distant compiler phases.

varungandhi-apple · 2020-07-23T22:48:29Z

I have a draft up here for incorporating Clang types in SIL: #33085.

hlopko · 2020-07-24T09:47:59Z

Hi Zoe,

Thanks for working on this! I chatted with @gribozavr a bit this morning and I'd like to discuss high-level bits before we dig into the code review.

I think instantiating function template signatures during SILGen is too late. We'll need to instantiate during the typechecking, at least templates that contain dependent types in the signature. Consider a function template:

template<class T>
void f(T t, std:: add_lvalue_reference<T>::type t_ref) { ... }

Before we substitute template arguments we cannot import this function into Swift and typecheck calls to it.

As for instantiating function template bodies, we see 2 options:

Instantiating bodies during typechecking right after instantiating signatures. This has the benefit of detecting and reporting instantiation errors in SourceKit and other IDE tooling that does not run IRGen.
Instantiating bodies as late as possible in IRGen. This has the benefit of avoiding unnecessary work (for example, when the call to the instantiation is inside unreachable code that gets deleted by SIL optimizer).

What do you think?

varungandhi-apple · 2020-07-24T14:35:01Z

Instantiating bodies during typechecking right after instantiating signatures. This has the benefit of detecting and reporting instantiation errors in SourceKit and other IDE tooling that does not run IRGen.

The IDE tooling part aspect sounds appealing; will this approach work with static_assert+constexpr and similar features which require complex compile time evaluation to type-check, or does that require operating on SIL/LLVM IR?

gribozavr · 2020-07-24T17:36:13Z

The IDE tooling part aspect sounds appealing; will this approach work with static_assert+constexpr and similar features which require complex compile time evaluation to type-check, or does that require operating on SIL/LLVM IR?

Instantiating the templates on the AST level is sufficient to get all errors. Clang does not produce errors from IRGen.

zoecarver · 2020-07-24T18:58:20Z

I think instantiating function template signatures during SILGen is too late. We'll need to instantiate during the typechecking, at least templates that contain dependent types in the signature.

SGTM. I'll work on re-factoring/writing this patch over the weekend so that it does the work during typechecking.

I think, especially, for now, we should do all the work in the type checker. For a few reasons:

Simplicity. I think it will be easier to implement, if we want to make it lazy in the future, that would be better as a follow up IMO.
Error handling (I'm not sure if there are any errors that only arise after synthesizing the function body).
Layering / decl storage. Where would we put the FunctionTemplateDecl? We'd need that in the IRGen phase but it wouldn't make sense to set it as any instructions' clangDecl.

@gribozavr also brought up a good point (which I was meaning to try and forgot), what happens when we have a function like this?

auto test() { return 0; }

The whole function probably needs to be synthesized in that case. I think this is another argument for doing all the work in the type checker.

zoecarver · 2020-08-10T19:50:13Z

I've updated this patch to have the specialization logic in type checking. This doesn't resolve the layering issue of having the ClangTypeConverter be public. The reason I think ClangTypeConverter may need to be public is this: the specialization of a function template must happen at earliest when the caller is instantiated. Otherwise, we don't know what types to substitute. The caller will never be instantiated in the clang importer so, this header is going to need to be accessed in multiple places. @CodaFi WDYT?

zoecarver · 2020-08-10T19:53:47Z

test/Interop/Cxx/function/template-irgen.swift

+// TODO: Generics supplied as template parameters.
+// X-CHECK-LABEL: define {{.*}}void @"$s4main22testPassThroughGeneric1xxx_tlF"(%swift.opaque* noalias nocapture sret %0, %swift.opaque* noalias nocapture %1, %swift.type* %T)
+// X-CHECK-LABEL: define linkonce_odr i8* @_Z11passThroughIP11objc_objectET_S2_(i8* %value)
+// public func testPassThroughGeneric<T>(x: T) -> T {


I think generic types should be a follow-up patch. The "issue" is that we need to generate a FuncDecl for the specialized clang function template with the specialized parameters. The specialized parameters are generic types so, they can't exist outside of the caller function. However, I think this can be solved pretty easily by just giving the new FuncDecl a generic parameter for each generic it gets from the caller.

zoecarver · 2020-08-10T19:58:44Z

The nice thing about having everything in the type checker is we now don't have to have any special SILGen or IRGen logic 🙂

hlopko

This is a great PR, thank you Zoe!

hlopko · 2020-08-12T12:09:22Z

include/swift/AST/ClangTypeConverter.h

@@ -78,9 +78,10 @@ class ClangTypeConverter :
  /// declaration introduced by this converter, and if so, return the original
  /// Swift declaration.
  Decl *getSwiftDeclForExportedClangDecl(const clang::Decl *decl) const;
+
+  clang::QualType convert(Type type);


+1 to not making ClangTypeConverter public.

@varungandhi-apple, did you mean to have a method that translates a templated C++ function type to a Swift function type on ClangModuleLoader? That sounds like a place that would keep ClangTypeConverter encapsulated and allow us to do what we need in this PR.

hlopko · 2020-08-12T12:29:28Z

lib/ClangImporter/ImportDecl.cpp

-                                      DeclContext *dc,
+                                      GenericParamList *genericParams,
+                                      ParameterList *bodyParams, Type resultTy,
+                                      bool throws, DeclContext *dc,


Disclaimer, I don't know what's the policy on reformatting in the Swift project.

I try to only reformat pieces of code directly modified by a PR. So I'd not reformat the whole ImportDecl.cpp (and other files) in this PR. I find the PR review easier this way, there are fewer distractions, and smaller chance of missing a behavioural change.

I'd instead upload a separate PR without behavioural changes and reformat for example the whole lib/AST or lib/ClangImporter + corresponding include directories. What do you think?

Please ignore this suggestion if you know what the policy is and this PR follows it.

I completely understand what you're saying. These parameters got reformatted because I added genericParams. But, I'll remove the header-re-arranging (or put it in another PR).

lib/ClangImporter/ImportDecl.cpp

hlopko · 2020-08-12T14:34:47Z

lib/Sema/CSApply.cpp

+                                                   clang::SourceLocation());
+  sema.InstantiateFunctionDefinition(clang::SourceLocation(), spec);
+  return spec;
+}


What do you think about moving this whole function as a method in ClangModuleLoader?

hlopko · 2020-08-12T14:36:31Z

lib/Sema/CSApply.cpp

@@ -578,7 +664,7 @@ namespace {
      if (!ref)
        ref = solution.resolveConcreteDeclRef(decl, loc);

-      assert(ref.getDecl() == decl);
+      // assert(ref.getDecl() == decl);


Remnants of a fun debugging session? :)

Actually, I think we need to remove this assertion. It seems to be checking that we in fact resolved a ConcreteDeclRef for the given decl and not a random one but, I don't think that's a necessary thing to check. If we get a different decl (as we do in this case) I think that's OK. And this is something that will easily be caught via unit tests if there's a logical error.

hlopko · 2020-08-12T14:39:36Z

test/Interop/Cxx/function/Inputs/function-templates.h

+}
+
+// Same here:
+template <class T> void cannot_infer_template() {}


Can we get some tests for SFINAE? For example showing that the right function out of an overload set is selected

Good idea. After writing some SFINAE tests I found an infinite loop. I'll have to fix that before this lands.

zoecarver · 2020-08-19T19:17:05Z

Sorry for the delay in updating this. I've removed the commit that makes ClangTypeConverter.h public.

lib/AST/ClangTypeConverter.h

lib/ClangImporter/ImportDecl.cpp

zoecarver · 2020-10-07T02:11:31Z

@swift-ci please test windows platform.

zoecarver · 2020-10-07T02:52:55Z

@swift-ci please smoke test.

zoecarver · 2020-10-07T02:54:22Z

Oops. I didn't think that would re-run the windows tests 🤦

zoecarver · 2020-10-09T19:18:07Z

include/swift/AST/ClangModuleLoader.h

@@ -219,6 +221,18 @@ class ClangModuleLoader : public ModuleLoader {
  /// based on subtleties like the target module interface format.
  virtual bool isSerializable(const clang::Type *type,
                              bool checkCanonical) const = 0;
+
+  virtual clang::FunctionDecl *
+  generateCXXFunctionTemplateSpecializations(ASTContext &ctx,


Rename to instantiateCXXFunctionTemplate.

zoecarver · 2020-10-12T04:21:42Z

@gribozavr friendly ping. Do you think you'll be able to review this patch in the next week or two? If not, what would you think about doing post-commit review?

zoecarver · 2020-10-19T06:22:56Z

Unless anyone has an objection, I'm going to land this patch on Tuesday.

zoecarver · 2020-10-21T18:29:29Z

There are some small changes (and lots of tests) that need to be made to support templated constructors. I'm going to implement that in a follow-up patch.

zoecarver · 2020-10-21T18:30:21Z

@swift-ci please test and merge.

zoecarver · 2020-10-21T18:36:50Z

lib/ClangImporter/ImportType.cpp

+      llvm::find_if(genericParams, [name](GenericTypeParamDecl *generic) {
+        return generic->getName().str() == name;
+      });
+  // TODO: once we support generics in class types, replace this with


@hlopko Just FYI, I think #33284 will start crashing once this lands. Happy to help resolve the issues if you'd like.

No worries, I'll get to that soon :)

zoecarver · 2020-10-21T18:37:19Z

@swift-ci please test and merge.

This patch adds rudimentary support for C++ template functions in swift.

zoecarver · 2020-10-21T23:33:14Z

@swift-ci please test.

zoecarver · 2020-10-21T23:39:22Z

@swift-ci please test windows platform.

zoecarver · 2020-10-21T23:39:35Z

@swift-ci please test Windows platform.

hlopko · 2020-10-25T18:57:38Z

Big congrats Zoe, great work!

include/swift/AST/ClangModuleLoader.h

include/swift/AST/ASTContext.h

test/Interop/Cxx/templates/Inputs/function-templates.h

gribozavr · 2020-10-05T15:28:20Z

test/Interop/Cxx/templates/Inputs/function-templates.h

+template <class T> const T passThroughConst(const T value) { return value; }
+
+void takesString(const char *) { }
+template <class T> void expectsString(T str) { takesString(str); }


expectsString => expectsConstCharPtr

Maybe expects CString?

lib/AST/ClangTypeConverter.h

lib/AST/ClangTypeConverter.cpp

test/Interop/Cxx/templates/function-template-errors.swift

zoecarver · 2021-01-13T20:51:18Z

@gribozavr Thank you for all those comments! Sorry they didn't make it into the original patch, but I've addressed them all in this PR: #35416.

… review comments from swiftlang#33053. Addresses the post-commit review comments from swiftlang#33053. Just re-naming, commenting, and some other small cleanups, nothing functionally different.

…e-post-commit [NFC] Cleanup function templates implementation. Address post-commit review comments from #33053.

zoecarver requested a review from gribozavr July 22, 2020 19:28

zoecarver added the c++ interop Feature: Interoperability with C++ label Jul 22, 2020

zoecarver force-pushed the cxx/templates branch from f2ccc33 to 852c823 Compare July 22, 2020 19:34

zoecarver mentioned this pull request Jul 22, 2020

[cxx-interop] [nfc] Make ClangTypeConverter.h a public header. #33054

Closed

zoecarver requested a review from hlopko July 22, 2020 19:35

CodaFi requested changes Jul 22, 2020

View reviewed changes

CodaFi reviewed Jul 22, 2020

View reviewed changes

CodaFi reviewed Jul 23, 2020

View reviewed changes

zoecarver force-pushed the cxx/templates branch from 852c823 to e4c070a Compare August 10, 2020 19:42

zoecarver commented Aug 10, 2020

View reviewed changes

hlopko reviewed Aug 12, 2020

View reviewed changes

zoecarver force-pushed the cxx/templates branch from e4c070a to 83fe483 Compare August 19, 2020 19:15

zoecarver requested review from hlopko and CodaFi August 19, 2020 19:16

zoecarver commented Aug 19, 2020

View reviewed changes

lib/AST/ClangTypeConverter.h Outdated Show resolved Hide resolved

zoecarver force-pushed the cxx/templates branch from 83fe483 to 5f1ee55 Compare August 19, 2020 19:19

zoecarver commented Aug 19, 2020

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved

zoecarver commented Oct 9, 2020

View reviewed changes

zoecarver mentioned this pull request Oct 16, 2020

Add support for calling C++ constructors #30630

Merged

zoecarver force-pushed the cxx/templates branch 2 times, most recently from 1b98f23 to ea397e0 Compare October 21, 2020 18:11

zoecarver force-pushed the cxx/templates branch from ea397e0 to c919f69 Compare October 21, 2020 18:30

zoecarver commented Oct 21, 2020

View reviewed changes

[cxx-interop] Support C++ function templates in Swift.

b2b6b05

This patch adds rudimentary support for C++ template functions in swift.

zoecarver force-pushed the cxx/templates branch from c919f69 to b2b6b05 Compare October 21, 2020 23:33

zoecarver merged commit f0f2246 into swiftlang:main Oct 22, 2020

zoecarver mentioned this pull request Nov 6, 2020

[cxx-interop] Support function templates inside a namespace. #34607

Merged

gribozavr reviewed Jan 13, 2021

View reviewed changes

zoecarver mentioned this pull request Jan 13, 2021

[NFC] Cleanup function templates implementation. Address post-commit review comments from #33053. #35416

Merged

zoecarver added a commit that referenced this pull request Jan 20, 2021

Merge pull request #35416 from zoecarver/cxx/cleanup-function-templat…

50f55c1

…e-post-commit [NFC] Cleanup function templates implementation. Address post-commit review comments from #33053.

[cxx-interop] Support function templates. #33053

[cxx-interop] Support function templates. #33053

Conversation

zoecarver commented Jul 22, 2020

CodaFi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varungandhi-apple commented Jul 23, 2020 • edited Loading

zoecarver commented Jul 23, 2020

varungandhi-apple commented Jul 23, 2020

varungandhi-apple commented Jul 23, 2020

hlopko commented Jul 24, 2020

varungandhi-apple commented Jul 24, 2020

gribozavr commented Jul 24, 2020

zoecarver commented Jul 24, 2020

zoecarver commented Aug 10, 2020

Choose a reason for hiding this comment

zoecarver commented Aug 10, 2020

hlopko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoecarver commented Aug 19, 2020

zoecarver commented Oct 7, 2020

zoecarver commented Oct 7, 2020

zoecarver commented Oct 7, 2020

Choose a reason for hiding this comment

zoecarver commented Oct 12, 2020

zoecarver commented Oct 19, 2020

zoecarver commented Oct 21, 2020

zoecarver commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoecarver commented Oct 21, 2020

zoecarver commented Oct 21, 2020

zoecarver commented Oct 21, 2020

zoecarver commented Oct 21, 2020

hlopko commented Oct 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoecarver commented Jan 13, 2021

CodaFi left a comment •

edited

Loading

varungandhi-apple commented Jul 23, 2020 •

edited

Loading