[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman edited 
https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman edited 
https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

The precommit CI failures are real; I'm still investigating what's going 
sideways.

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[via cfe-commits]

@@ -1163,7 +1137,7 @@ class Sema;
 ConversionSequenceList
 allocateConversionSequences(unsigned NumConversions) {
   ImplicitConversionSequence *Conversions =
-  slabAllocate(NumConversions);
+  Ctx.Allocate(NumConversions);
 
   // Construct the new objects.
   for (unsigned I = 0; I != NumConversions; ++I)

cor3ntin wrote:

Maybe something like that would be cleaner

```cpp
 ConversionSequenceList allocateConversionSequences(unsigned NumConversions) {
  ImplicitConversionSequence *Conversions = 
Ctx.Allocate(NumConversions);
  std::uninitialized_default_construct(Conversions, Conversions + 
NumConversions);
  return ConversionSequenceList(Conversions, NumConversions);
}
```

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Matheus Izvekov via cfe-commits]

@@ -1070,57 +1077,24 @@ class Sema;
 };
 
   private:
-SmallVector Candidates;
-llvm::SmallPtrSet Functions;
-
-// Allocator for ConversionSequenceLists. We store the first few of these
-// inline to avoid allocation for small sets.
-llvm::BumpPtrAllocator SlabAllocator;
+ASTContext &Ctx;
+SmallVector Candidates;
+llvm::SmallPtrSet Functions;

mizvekov wrote:

pre-existing small nit, for consistency:
```suggestion
SmallVector Candidates;
SmallPtrSet Functions;
```

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Aaron Ballman via cfe-commits]

@@ -1070,57 +1077,24 @@ class Sema;
 };
 
   private:
-SmallVector Candidates;
-llvm::SmallPtrSet Functions;
-
-// Allocator for ConversionSequenceLists. We store the first few of these
-// inline to avoid allocation for small sets.
-llvm::BumpPtrAllocator SlabAllocator;
+ASTContext &Ctx;
+SmallVector Candidates;
+llvm::SmallPtrSet Functions;

AaronBallman wrote:

The reason it works to drop the `llvm::` prefix for `SmallVector` is because we 
do this: 
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/LLVM.h

`SmallPtrSet` is not included in there which is why the prefix is necessary, 
and I'm not certain it's worth including it or not.

Thoughts?

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[via cfe-commits]

https://github.com/cor3ntin approved this pull request.

This was a roller coaster!
LGTM

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Matheus Izvekov via cfe-commits]

@@ -1070,57 +1077,24 @@ class Sema;
 };
 
   private:
-SmallVector Candidates;
-llvm::SmallPtrSet Functions;
-
-// Allocator for ConversionSequenceLists. We store the first few of these
-// inline to avoid allocation for small sets.
-llvm::BumpPtrAllocator SlabAllocator;
+ASTContext &Ctx;
+SmallVector Candidates;
+llvm::SmallPtrSet Functions;

mizvekov wrote:

I see, it's fine either way 😃 

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Erich Keane via cfe-commits]

https://github.com/erichkeane approved this pull request.


https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Aaron Ballman via cfe-commits]

https://github.com/AaronBallman closed 
https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Vitaly Buka via cfe-commits]

vitalybuka wrote:

A new memory leak https://lab.llvm.org/buildbot/#/builders/5/builds/42694

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Nikita Popov via cfe-commits]

nikic wrote:

This change seems to cause significant compile-time regressions for C++ code 
(https://llvm-compile-time-tracker.com/compare.php?from=184ba038ac1d444980b3e554b0057f3f30c516ab&to=4082a7554521572a65a5a0008c4661a534df659d&stat=instructions%3Au).

Probably most damning are the times for the clang build itself 
(https://llvm-compile-time-tracker.com/compare_clang.php?from=184ba038ac1d444980b3e554b0057f3f30c516ab&to=4082a7554521572a65a5a0008c4661a534df659d&stat=instructions%3Au&sortBy=interestingness)
 where files like X86ISelDAGToDAG.cpp regress by >3% even in an optimized build.

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Vitaly Buka via cfe-commits]

vitalybuka wrote:

have ideas about leak, but not sure what to do with perf regression and #88330

Proposing revert https://github.com/llvm/llvm-project/pull/89006 

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Vitaly Buka via cfe-commits]

@@ -1163,12 +1136,7 @@ class Sema;
 ConversionSequenceList
 allocateConversionSequences(unsigned NumConversions) {
   ImplicitConversionSequence *Conversions =
-  slabAllocate(NumConversions);
-
-  // Construct the new objects.
-  for (unsigned I = 0; I != NumConversions; ++I)
-new (&Conversions[I]) ImplicitConversionSequence();
-
+  new ImplicitConversionSequence[NumConversions];

vitalybuka wrote:

Seems unusual to keep ownership in ArrayRef.
Could we make ConversionSequenceList just a std::vector if it owns the memory?

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[via cfe-commits]

cor3ntin wrote:

@AaronBallman ping


https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[Aaron Ballman via cfe-commits]

AaronBallman wrote:

> @AaronBallman ping

I've not been able to get back on to this, unfortunately. I'm not certain when 
I'll have time, either.

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] Improve stack usage to increase recursive initialization depth (PR #88546)

[via cfe-commits]

cor3ntin wrote:

It might be worth reopening the PR and asking for help? otherwise we'll forget

*And some things that should not have been forgotten were lost.*

https://github.com/llvm/llvm-project/pull/88546
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits