[clang] Template Specialization Resugaring - TypeDecl

[mizvekov]

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 <[e-mail]>

[Mordante]

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]

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]

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]

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]

clang/lib/Sema/SemaTemplate.cpp

533–539

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

[davrec]

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?

1001–1007

Move back up?

clang/lib/Sema/SemaExpr.cpp

3427–3430 ↗

(On Diff #464192)

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

20800 ↗

(On Diff #464192)

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

clang/lib/Sema/SemaExprMember.cpp

1862 ↗

(On Diff #464192)

Rename to FieldQuals

clang/lib/Sema/SemaOverload.cpp

15357 ↗

(On Diff #464192)

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

4345–4347

Change back to original?

4956–4971

Move back up?

clang/lib/Sema/TreeTransform.h

2679–2681 ↗

(On Diff #464192)

Move back down?

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

244 ↗

(On Diff #449993)

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

[mizvekov]

In D127695#3829020, @davrec wrote:

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.

[rjmccall]

What is this work about?

[mizvekov]

In D127695#3860237, @rjmccall wrote:

What is this work about?

I have written more about it in this discourse thread: https://discourse.llvm.org/t/rfc-improving-diagnostics-with-template-specialization-resugaring/64294

[rjmccall]

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]

In D127695#3860316, @rjmccall wrote:

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.