Instead of building the definition of a thunk immediately when we
generate the thunk declaration, wait until we reach the `}` of the
outermost class, interface, etc. -- at the same time when we would parse
the definition of the thunk if it were defined inline.
This fixes issues where we fail to define the thunk because it requires
an enclosing class to be complete, or its definition depends on
something declared later in the enclosing class.
Make the representation of a suspended function scope, and its
constituent suspended components, be move-only, and switch to passing it
around by rvalue reference instead of by value because it's expensive
both to move and especially to copy.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
This is essentially the result of looking at `.begin()` uses. We also
frequently do `std::shuffle`, but unfortunately STLExtras doesn't
provide a wrapper for that.
This converts `StructTypeField` from an instruction to a dedicated type,
with its own store. This had originated from discussing how
`.GetAs<SemIR::StructTypeField>` was more prevalent than for other
instructions, but is probably more interesting for the storage savings
(16 bytes StructTypeField + 4 byte LocId + 4 byte InstId -> 8 byte
StructTypeField).
Due to the different structure, these now have their own stack during
construction, reducing (but not eliminating) `args_type_info_stack_`
use-cases.
The test changes of different InstIds is expected because structs and
classes generate fewer instructions now. Other than that, results should
remain the same.
I'm generally trying to avoid unrelated cleanup here due to the PR size,
though I did scrutinize the `VerifyOnFinish` calls, adding one and
commenting others (putting them in member order because that's how I was
checking what was verified and what wasn't).
A small step to virtual functions - adding vtable pointers to the
layout, but not initializing or otherwise using them at this stage.
A few open design questions I'd love feedback on:
* Is this the right/good enough SemIR representation for now? This patch
adds a `is_dynamic` attribute to `SemIR::Class` and populates/flags it
based on the flag of the base class, or if any virtual function is
declared in the class (or, at least that's my intent). Some other
options include:
* Each `Class` could store a `ClassId` (or `TypeId`?) of the (possibly
indirect, possibly self) base class that is the first one that is
dynamic/has a vtable pointer
* Could make the property narrower, like `has vtable pointer` and have
it `true` only on the type that introduces the vtable - then derived
classes would have to walk their base classes to check if they're the
one that needs to define the vtable pointer or not
* Should the vtable be the first element in the type? If there's a
non-dynamic base type, we could have a layout that's `{<non-dynamic base
type>, vtable ptr, <derived members>}`? Derived types would still be
able to uniquely identify where their vtable pointer is just fine... -
and the vtable pointer is, in a sense, a member of that intermediate
type, so it does seem a bit strange to force it to the front - but I
guess it's probably more efficient in some ways?
Open to any other suggestions/advice/thoughts on the direction, etc.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This switches `DCHECK` and `FATAL` as well.
The goal is to reduce the code size impact of these assertions so that
we can keep more of them enabled. Currently, the largest cost I see from
`CHECK` is not the actual check or the cold code itself, but actually
the failure to inline trivial functions due to the presence of the cold
code. This means that our goal isn't to reduce apparent code size in the
final binary but the LLVM IR cost assessed for these routines in the
inliner, which closely correlates with code size but is a bit different.
As discussed in #4283, experimentation shows that a single function call
with a minimal number of arguments is the lowest cost model for these.
This is easily achieved with a format-string API that internally uses
`llvm::formatv`. This PR is essentially the `CHECK` version of #4283.
However, the check macros are substantially harder to make work with
both format strings and streaming because they also take a condition.
Also, unexpectedly, I was very successful at devising a regular
expression based automated rewrite from the streaming to the format
string form with only low 10s of manual fixes. This includes compacting
strings broken up across lines, etc. Given how well that went, I've
prepared this PR which just directly switches to the format string API
and migrate everything to use it.
One nice side-effect is that the format string approach ends up greatly
simplifying the implementation here as well.
This is ... *shockingly* effective. Parsing speeds up by more than 3%
with just this change. And checking speeds up by **8%** with this change
alone:
```
BM_CompileAPIFileDenseDecls<Phase::Parse>/256 86.3µs ± 1% 82.9µs ± 1% -3.94% (p=0.000 n=17+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/1024 431µs ± 1% 415µs ± 1% -3.76% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/4096 1.77ms ± 1% 1.71ms ± 1% -3.18% (p=0.000 n=18+19)
BM_CompileAPIFileDenseDecls<Phase::Parse>/16384 7.44ms ± 1% 7.17ms ± 2% -3.56% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/65536 30.7ms ± 1% 29.7ms ± 1% -3.15% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Parse>/262144 131ms ± 1% 127ms ± 1% -2.81% (p=0.000 n=18+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/256 878µs ± 2% 800µs ± 1% -8.91% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/1024 1.88ms ± 2% 1.72ms ± 1% -8.56% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/4096 5.78ms ± 2% 5.28ms ± 1% -8.70% (p=0.000 n=20+18)
BM_CompileAPIFileDenseDecls<Phase::Check>/16384 21.9ms ± 1% 20.1ms ± 1% -8.02% (p=0.000 n=18+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/65536 90.4ms ± 2% 83.1ms ± 1% -8.04% (p=0.000 n=19+20)
BM_CompileAPIFileDenseDecls<Phase::Check>/262144 381ms ± 2% 352ms ± 1% -7.79% (p=0.000 n=19+19)
```
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: josh11b <15258583+josh11b@users.noreply.github.com>
The use of ArrayStack here is intended to simplify the logic, and also
make better use of the inst heap allocations. Prior changes #4101 and
#4103 removed the less related logic from InstBlockStack, although #4103
is the actual part that blocked using ArrayStack.
BTW, note the PrintForStackDump implementation was incorrect because it
didn't apply size_. This simplification fixes the issue.
Based on discussion around the region handling in generic_region_stack,
create a generic structure for the stack-of-vectors support. I also want
to add this to InstBlockStack, but that's a little more complex due to
GlobalInit, so cutting a PR here to check with review.
My work here is how I noticed #4099; I want to be sure that I'm correct
about the issue, but it's the difference between being able to use
PeekArray or not.
Note in scope_stack.h, I believe we could remove next_compile_time_index
and make it just based on elements_size(). However, I want to verify
with you before I make further changes there.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Jon Ross-PerkinsandRichard Smith
Boaz Brickner
Geoff RomerandRichard Smith
David BlaikieandRichard Smith
