This is an archive of the discontinued LLVM Phabricator instance.

[clang] Template Specialization Resugaring - TypeDecl
Needs ReviewPublic

Authored by mizvekov on Jun 13 2022, 1:59 PM.

Details

Reviewers
shafik
aaron.ballman
NoQ
Group Reviewers
Restricted Project
Summary

This is the introductory patch for a larger work which
intends to solve the long standing C++ issue with losing
type sugar when acessing template specializations.

The well known example here is specializing a template
with std::string, but getting diagnostics related to
std::basic_string<char> instead.

This implements a transform which, upon member access,
propagates type sugar from the naming context into the
accessed entity.

It also implements a single use of this transform,
resugaring access to TypeDecls.

For more details and discussion see:
https://discourse.llvm.org/t/rfc-improving-diagnostics-with-template-specialization-resugaring/64294

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mizvekov planned changes to this revision.Jun 30 2022, 6:37 PM
mizvekov updated this revision to Diff 441569.
mizvekov retitled this revision from clang: Implement Template Specialization Resugaring to WIP: clang: Implement Template Specialization Resugaring.
mizvekov edited the summary of this revision. (Show Details)
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added a subscriber: cfe-commits. · View Herald Transcript
mizvekov updated this revision to Diff 443473.Jul 9 2022, 8:10 PM
mizvekov planned changes to this revision.Jul 9 2022, 8:10 PM
mizvekov planned changes to this revision.Jul 22 2022, 5:28 PM
mizvekov updated this revision to Diff 447012.
mizvekov requested review of this revision.Jul 24 2022, 9:01 AM
mizvekov updated this revision to Diff 447139.
mizvekov retitled this revision from WIP: clang: Implement Template Specialization Resugaring to clang: Implement Template Specialization Resugaring.
mizvekov planned changes to this revision.Jul 24 2022, 9:02 AM
mizvekov retitled this revision from clang: Implement Template Specialization Resugaring to WIP: clang: Implement Template Specialization Resugaring.
mizvekov updated this revision to Diff 447828.Jul 26 2022, 2:11 PM
mizvekov planned changes to this revision.Jul 26 2022, 2:11 PM
mizvekov updated this revision to Diff 448009.Jul 27 2022, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 27 2022, 5:29 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
mizvekov planned changes to this revision.Jul 27 2022, 5:29 AM
mizvekov planned changes to this revision.Jul 27 2022, 2:02 PM
mizvekov updated this revision to Diff 448155.
mizvekov planned changes to this revision.Aug 4 2022, 8:35 AM
mizvekov updated this revision to Diff 449993.
davrec added a subscriber: davrec.Aug 10 2022, 6:28 AM
davrec added inline comments.
clang/test/Sema/Resugar/resugar-expr.cpp
245

Compositions of MemberExprs/CXXMemberCallExprs have an issue:

template <class A1> struct A {
  struct Inner {
    A1 m;
    A1 f();
  } inner;
  Inner g();
};
Z x1 = A<Int>().inner.m; //No resugar
Z x2 = A<Int>().inner.f(); //No resugar
Z x3 = A<Int>().g().m; //No resugar
Z x4 = A<Int>().g().f(); //No resugar
Z x5 = A<Int>::Inner().m; //ok

Composed CallExprs seem to work but probably warrant a test, e.g.

template <class B1> B1 h(B1);
Z x6 = h(Int());
Z x7 = h(h(Int()));

https://godbolt.org/z/cszrsvh8d

mizvekov added inline comments.Aug 10 2022, 8:25 AM
clang/test/Sema/Resugar/resugar-expr.cpp
245

Thanks for the feedback!

The problem here I think is that for a MemberExpr, we have to look at not only the BaseType of it for a resugaring context, but instead look at its BaseExpr to see if it or any bases of it, and so on recursively, can provide a resugaring context as well, effectively building one naming context from the whole composition.

I will try to address that later, I probably should work on facilitating the reviews of the other patches in this stack, and then splitting this one big patch up more :)

nridge added a subscriber: nridge.Aug 27 2022, 9:15 PM
mizvekov planned changes to this revision.Aug 28 2022, 5:06 PM
mizvekov updated this revision to Diff 456221.
mizvekov planned changes to this revision.Aug 28 2022, 5:12 PM
mizvekov updated this revision to Diff 456222.
mizvekov edited the summary of this revision. (Show Details)
mizvekov planned changes to this revision.Aug 29 2022, 2:58 AM
mizvekov updated this revision to Diff 456295.
mizvekov updated this revision to Diff 456333.Aug 29 2022, 7:18 AM
mizvekov planned changes to this revision.Aug 29 2022, 7:18 AM
mizvekov planned changes to this revision.Aug 29 2022, 8:41 AM
mizvekov updated this revision to Diff 456358.
mizvekov planned changes to this revision.Aug 29 2022, 8:59 AM
mizvekov updated this revision to Diff 456363.
mizvekov requested review of this revision.Aug 29 2022, 11:07 AM
mizvekov updated this revision to Diff 456414.
mizvekov retitled this revision from WIP: clang: Implement Template Specialization Resugaring to clang: Implement Template Specialization Resugaring.
mizvekov planned changes to this revision.Aug 29 2022, 11:26 AM
mizvekov updated this revision to Diff 456425.
mizvekov retitled this revision from clang: Implement Template Specialization Resugaring to WIP: clang: Implement Template Specialization Resugaring.
mizvekov updated this revision to Diff 456461.Aug 29 2022, 2:24 PM
mizvekov planned changes to this revision.Aug 29 2022, 2:24 PM
mizvekov planned changes to this revision.Aug 29 2022, 4:15 PM
mizvekov updated this revision to Diff 456489.
Mordante added inline comments.
libcxx/utils/ci/buildkite-pipeline.yml
377 ↗(On Diff #456489)

I actually think this change would be nice for libc++ in general. I've has several assertion failures in the bootstrapping CI. Especially with the symbolizer available the crash reports become a lot better.

I'm not convinced the way you used the CXXFLAGS in the CMakeLists.txt is the best way, maybe @ldionne has a better suggestion.

mizvekov added inline comments.Aug 30 2022, 12:17 PM
libcxx/utils/ci/buildkite-pipeline.yml
377 ↗(On Diff #456489)

Ah, this is just an idea at this point, this is not even working for crashes in lit-tests, the LIBCXX_TEST_COMPILER_FLAGS setting below did not seem to work.

I would really appreciate suggestions, but meanwhile I'll try to get something minimally working here and spin a separate DR.

davrec added inline comments.
clang/lib/Sema/SemaTemplate.cpp
533–539

Haven't thought this through fully, but: would the following make D128113 (storing the pack index in the STTPT or introducing a new sugar type) unnecessary?

map<pair<Decl *, unsigned>, Optional<unsigned>> CurPackIndices;
QualType TransformSubstTemplateTypeParmType(TypeLocBuilder &TLB,
                                              SubstTemplateTypeParmTypeLoc TL) {
   QualType QT = TL.getType();
   const SubstTemplateTypeParmType *T = TL.getTypePtr();
   Decl *ReplacedDecl = T->getReplacedDecl();
   
   Optional<unsigned> &PackIndex = CurPackIndices[{ReplacedDecl, T->getIndex()}];
   if (!PackIndex && T->getReplacedParameter()->isParameterPack())
     PackIndex = 0;
    
   ...

   if (PackIndex)
     ++PackIndex;
     // ^ maybe reset to zero if > pack size, if we might be resugaring multiple expansions
   return QT;
}
davrec added inline comments.Sep 8 2022, 8:05 AM
clang/lib/Sema/SemaTemplate.cpp
533–539

Disregard above - upon further thought this does not improve the things; there still isn't enough info about the substitutions. I.e. the issue is with substitutions, not the parameter declarations for which they are substituted. So a sugar node wrapping the STTPTs to represent each expansion instance really is needed. Then when we have that I think we could map from those to their current pack indices per the above to infer the pack indices.

For this sugar node, maybe we could just modify SubstTemplateTypeParmPackType so it is not just canonical, but can also be sugar wrapping substituted STTPTs, as opposed to introducing a new Subst* type class?

mizvekov marked 2 inline comments as done.Sep 8 2022, 8:24 AM
mizvekov added inline comments.
clang/lib/Sema/SemaTemplate.cpp
533–539

I will reply to this on https://reviews.llvm.org/D128113.

mizvekov planned changes to this revision.Sep 14 2022, 1:54 PM
mizvekov updated this revision to Diff 460214.
mizvekov marked an inline comment as done.
mizvekov planned changes to this revision.Sep 14 2022, 3:28 PM
mizvekov updated this revision to Diff 460253.
mizvekov requested review of this revision.Sep 15 2022, 6:11 PM
mizvekov updated this revision to Diff 460596.
mizvekov retitled this revision from WIP: clang: Implement Template Specialization Resugaring to clang: Implement Template Specialization Resugaring.
mizvekov updated this revision to Diff 460912.Sep 16 2022, 2:34 PM
mizvekov edited the summary of this revision. (Show Details)
mizvekov updated this revision to Diff 461021.Sep 17 2022, 10:52 AM
mizvekov retitled this revision from clang: Implement Template Specialization Resugaring to [clang] Implement Template Specialization Resugaring.
mizvekov edited the summary of this revision. (Show Details)
mizvekov updated this revision to Diff 461031.Sep 17 2022, 3:43 PM
mizvekov updated this revision to Diff 461093.Sep 18 2022, 12:58 PM
mizvekov updated this revision to Diff 461972.Sep 21 2022, 11:45 AM
mizvekov updated this revision to Diff 461992.Sep 21 2022, 1:14 PM
mizvekov updated this revision to Diff 462029.Sep 21 2022, 3:40 PM
mizvekov updated this revision to Diff 462176.Sep 22 2022, 8:08 AM
mizvekov updated this revision to Diff 464192.Sep 30 2022, 2:32 AM
mizvekov updated this revision to Diff 464436.Sep 30 2022, 4:41 PM
davrec added a subscriber: rjmccall.Oct 1 2022, 5:57 AM

First thank you for having separated out the public AST changes into other patches, it makes these mostly-Sema changes much easier to review.

I don't see any major issues with the code, though this would benefit from a close look from more folks with deeper Sema familiarity. Maybe @rjmccall would be willing or could @ someone?

The larger issues:

  1. Performance - can we see some numbers? and
  2. There are a lot of FIXMEs introduced - understandable because of the scale, but it would be nice to hear you commit to eventually getting to those because otherwise they will probably remain indefinitely. @aaron.ballman should probably weigh in/sign off on this one given the number of FIXMEs, which probably can't be handled before @mizvekov 's deadline (when is that again?).
clang/lib/Sema/SemaCXXScopeSpec.cpp
881–884

Move back down?

997–1003

Move back up?

clang/lib/Sema/SemaExpr.cpp
3436–3439

Does this need the same changes as Sema::FixOverloadedFunctionReference? (see below)

20635

Could this be done same as earlier (lines 19548-19553)?

clang/lib/Sema/SemaExprMember.cpp
1858

Rename to FieldQuals

clang/lib/Sema/SemaOverload.cpp
15294–15295

I think this should have said "Duplicated from diagnoseUncapturableValueReferenceOrBinding" - if so do we need the below changes over there too?

clang/lib/Sema/SemaTemplate.cpp
94

static/move to namespace {} below. Or make it a public method of Decl? Decl::getOriginalDescribedTemplate() or something like that? (If you go that route then best to fall back to getDescribedTemplate instead of the unreachable.)

129

Not sure of policy, should this dump should be removed? Several others below too.

160

Wouldn't hurt to add a brief comment above each RAII class describing its role

256

Would be nice to have a comment explaining the effect of Reverse here or above addTypeToMap. (Reverse iteration?)

264

Could clarify to CXXRecordDecl

292

Add message

465

Maybe include T->OwnedTagDecl arg for consistency with deduction resugaring

478

AssociatedDecl

742

Add message

4703–4705

Change back to original?

5290–5305

Move back up?

clang/lib/Sema/TreeTransform.h
2683–2685

Move back down?

clang/test/Sema/Resugar/resugar-expr.cpp
245

Inheritance is also an issue:

https://godbolt.org/z/81nPqzsrd

using Int = int;
enum class Z;

template<typename A1>
struct A {
    A1 m;
};

template<typename A1>
struct B : A<A1> {};

Z x2 = B<Int>().m; //doesn't resugar
martong removed a subscriber: martong.Oct 3 2022, 2:13 AM
mizvekov updated this revision to Diff 466369.Oct 9 2022, 5:45 AM
mizvekov updated this revision to Diff 466376.Oct 9 2022, 8:43 AM
mizvekov edited the summary of this revision. (Show Details)
mizvekov marked 9 inline comments as done.

The larger issues:

  1. Performance - can we see some numbers? and

There are still work left to do on a couple of enablers, and some other changes, in order to get performance on a level which is meaningful to discuss.
The next bit of work here is to get rid of deducing partial specializations during resugaring, which hits some test cases specially bad.

  1. There are a lot of FIXMEs introduced - understandable because of the scale, but it would be nice to hear you commit to eventually getting to those because otherwise they will probably remain indefinitely.

From the MR desciption, I am sorry that it's not clear that this is not finished and not really ready for review at that level of detail.

So I am leaving some dumps and other stuff meanwhile, but will clean this up when it's almost there. And I will add the much needed patch description.

The tool that I am using to handle phabricator (moz-phab) is really finicky about leaving the "Changes planned" status, so this was lost here.
I addressed most of your comments though.
For the other comments, some other work has to be finished first in order to address them.

@aaron.ballman should probably weigh in/sign off on this one given the number of FIXMEs, which probably can't be handled before @mizvekov 's deadline (when is that again?).

I am not counting anymore that I will have everything merged by the beginning of November. Don't worry too much about deadlines, we have done a lot as part of the GSoC already, much thanks to you for helping push the reviews :)
The work will continue afterwards in any case.

clang/lib/Sema/SemaTemplate.cpp
94

This function will disappear soon, after a few other changes.

mizvekov marked an inline comment as done.Oct 9 2022, 8:45 AM
mizvekov updated this revision to Diff 467924.Oct 14 2022, 2:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2022, 2:32 PM

What is this work about?

I see, thank you. I know you've marked this a WIP, but it's always good to describe these things in the patch description; that's what it's for.

This is quite exciting! Thank you for looking into this.

I think you could usefully be much more incremental here. Obviously the resugaring transform is one big piece, but it's likely that you could usefully roll out the applications of it one at a time and see some incremental improvements, and it would be a lot easier for us to then review the changes you're needing to make to see why you're making them. That shouldn't make it any more difficult to measure holistic things like the overall compile-time impact of resugaring — it's all filtering through a single place, so you ought to be able to easily disable it there. You could also disable it by default by leaving it behind a -cc1 flag that you only enable in your tests. That would let you actually land the work incrementally, which I would have no objection to.

mizvekov updated this revision to Diff 472416.Nov 1 2022, 2:59 PM
mizvekov retitled this revision from [clang] Implement Template Specialization Resugaring to [clang] Template Specialization Resugaring - TypeDecl.
mizvekov edited the summary of this revision. (Show Details)

I see, thank you. I know you've marked this a WIP, but it's always good to describe these things in the patch description; that's what it's for.

Done! I have also removed the WIP. While there is still ongoing work with precursors of this patch and it might reflect in some changes here, I don't think anything significant will change, and this is good for review now.

This is quite exciting! Thank you for looking into this.

Thanks!

I think you could usefully be much more incremental here. Obviously the resugaring transform is one big piece, but it's likely that you could usefully roll out the applications of it one at a time and see some incremental improvements, and it would be a lot easier for us to then review the changes you're needing to make to see why you're making them. That shouldn't make it any more difficult to measure holistic things like the overall compile-time impact of resugaring — it's all filtering through a single place, so you ought to be able to easily disable it there. You could also disable it by default by leaving it behind a -cc1 flag that you only enable in your tests. That would let you actually land the work incrementally, which I would have no objection to.

Done.

I have split the resugaring applications in three parts for now.

I consider this patch and the next one to be a single application of the transform, each.

re your concerns and earlier question from @davrec regarding performance, right now we are regressing a couple of test cases in llvm-compile-time-tracker by about 1%:

 		Commit           kimwitu++            Bullet              tramp3d-v4          7zip                 geomean
C 		29cd42cab9       49485M (+0.23%)      112040M (+0.05%)    109687M (+0.46%)    227685M (+0.17%)     73023M (+0.10%)
C 		252f292ed2       49371M (+0.50%)      111980M (+0.11%)    109182M (+0.50%)    227293M (+0.98%)     72952M (+0.21%)

Showing this patch on the top, and the previous one on the bottom. I have omitted most test cases as they show essentially no difference.
These are number for optimized (O3) builds.

So most of our hit is coming from the previous patch, which I am working to improve. And I think there is still a lot of things that can be tried on this patch to get that number even lower.

Applying all the rest of the resugaring patches, up to the third one, increases the worst case by an additional 0.25%. Most test cases still show essentially no change.

You can see these numbers at http://llvm-compile-time-tracker.com/?config=NewPM-O3&stat=instructions%3Au&remote=mizvekov.

I am inclined to think that, for now, we will not need to offer an option to turn resugaring off, on performance grounds.