[llvm-branch-commits] [clang] [serialization] No transitive type change (PR #92511)

[Chuanqi Xu via llvm-branch-commits]

ChuanqiXu9 wrote:

> Thanks for the change! I have done a round of review and left a few suggestion and also have a bunch of questions. I am sorry if some of them may look too obvious, I did try to dig into the code where I could, but Clang's serialization is complicated and some things are easier researched through a conversation than through looking at code. Please bear with me, I promise to get better in the upcoming reviews.

You're welcome.

> 
> Here are a few extra question that came to mind too.
> 
> Question 1: Do we have estimates on how much bigger the PCMs become? Did you observer any negative performance impact?
> 
> I would expect `TypeID`s to be much more common than decls, and the corresponding size increases to be more significant. We've already lost ~6% from DeclID change, I am slightly worried the types are going to be a bigger hit.

I did and my local results shows, I see less than 1% change with this patch. This fits my understanding somehow. Since the `TypeID` is much less common than `DeclID`. This matches my experience in coding. The `DeclID` patch is the hardest one and the `TypeID` patch is the easiest one.

> 
> Question 2: could you explain why we the PCM in your example was changing before? Was there some base offset inherited from the PCM files we depended on?

Yes. In the higher level, it should be easy to understand. There is a new type in `m-partA.v1.cppm`. And in the low level, when we write a module file, we would record the type offset for the module files we depend on: https://github.com/llvm/llvm-project/blob/ad79a14c9e5ec4a369eed4adf567c22cc029863f/clang/lib/Serialization/ASTWriter.cpp#L5454

And this is exactly what the series patch want to eliminate.

https://github.com/llvm/llvm-project/pull/92511