[PATCH] D41416: [modules] [pch] Do not deserialize all lazy template specializations when looking for one.

[Vassil Vassilev via Phabricator via cfe-commits]

v.g.vassilev added a comment.

In D41416#4496389 <https://reviews.llvm.org/D41416#4496389>, @ChuanqiXu wrote:

> In D41416#4496293 <https://reviews.llvm.org/D41416#4496293>, @v.g.vassilev wrote:
>
>> In D41416#4492367 <https://reviews.llvm.org/D41416#4492367>, @v.g.vassilev wrote:
>>
>>> Address most of the comments. I will need some help with the `Modules/pr60085.cppm` failure. I suspect we pass to CodeGen some instantiation by iterator index and that does not work as the instantiations are lazily triggered upon use now.
>>
>> In fact it looks like it fails exactly in the same way as described in the original report https://github.com/llvm/llvm-project/issues/60085. The commit message in https://github.com/llvm/llvm-project/commit/78e48977a6e67 hints at the fact that the issue was gone surprisingly. I suspect that test case stopped reproducing the underlying issue by chance. That probably means that this patch is not breaking anything but exposing an underlying problem...
>
> Got it. I understand it is frustrating to fix an additional bug during development. Also it is generally not good to revert a valid test case.

I agree. Here is a reduced version of the same test case:

  // RUN: rm -rf %t
  // RUN: mkdir %t
  // RUN: split-file %s %t
  //
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/d.cppm \
  // RUN:     -emit-module-interface -o %t/d.pcm
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/c.cppm \
  // RUN:     -emit-module-interface -o %t/c.pcm -fmodule-file=%t/d.pcm
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/b.cppm \
  // RUN:     -emit-module-interface -o %t/b.pcm -fmodule-file=%t/d.pcm
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/a.cppm \
  // RUN:     -emit-module-interface -o %t/a.pcm -fmodule-file=%t/d.pcm \
  // RUN:     -fmodule-file=%t/c.pcm -fmodule-file=%t/b.pcm 
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/a.pcm \
  // RUN:     -S -emit-llvm -disable-llvm-passes -o - | FileCheck %t/a.cppm
  //
  // Use -fmodule-file=<module-name>=<BMI-path>
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/d.cppm \
  // RUN:     -emit-module-interface -o %t/d.pcm
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/c.cppm \
  // RUN:     -emit-module-interface -o %t/c.pcm -fmodule-file=%t/d.pcm
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/b.cppm \
  // RUN:     -emit-module-interface -o %t/b.pcm -fmodule-file=%t/d.pcm
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/a.cppm \
  // RUN:     -emit-module-interface -o %t/a.pcm -fmodule-file=%t/d.pcm \
  // RUN:     -fmodule-file=%t/c.pcm -fmodule-file=%t/b.pcm 
  // RUN: %clang_cc1 -std=c++20 -triple %itanium_abi_triple %t/a.pcm \
  // RUN:     -S -emit-llvm -disable-llvm-passes -o - | FileCheck %t/a.cppm
  
  //--- d.cppm
  export module d;
  
  export template<typename>
  struct integer {
    using type = int;
  
    static constexpr auto value() {
      return 0;
    }
  
  };
  
  export constexpr void ddd(auto const v) {
    v.value();
  }
  
  //--- c.cppm
  export module c;
  
  import d;
  
  int use = integer<int>().value();
  
  //--- b.cppm
  export module b;
  
  import d;
  
  using use = integer<int>::type;
  
  //--- a.cppm
  export module a;
  
  import d;
  
  export void a() {
    ddd(integer<int>());
  }



> Are you in a hurry to land this recently? (e.g., land this for clang17) If not, I suggest to wait for me to fix the underlying issue after clang17 gets released. And if I take too long to fix it (e.g, 9.30), given the speciality of the patch, I think we can revert that test case and land this one that time. How do you feel about the idea?

We are not in hurry - this patch was developed in 2017 :) It can probably wait a few more years ;) I had some spare cycles to work on it. That's all.

I do not think we can land it without asking @rsmith or @dblaikie to test it on their internal builds to make sure things are not regressing performance...


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

https://reviews.llvm.org/D41416