[libcxx-commits] [PATCH] D127695: [clang] Template Specialization Resugaring - TypeDecl

Tue Nov 1 16:24:53 PDT 2022

mizvekov added a comment.

In D127695#3860316 <https://reviews.llvm.org/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 <https://reviews.llvm.org/owners/package/3/>) 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127695