If a keyword or a sized type literal (eg, `f2`) is used in a context
where we are confident that we are expecting an identifier -- either
before a `:` in a binding pattern or after a `.` in a member access or
designator -- then recover as if a raw identifier was used.
This appears to be a particular stumbling block for coding agents, so
seems worth paying special attention to.
Add a mechanism to the tokenized buffer to track additional tokens
synthesized for error recovery so that we can keep the lexed token
sequence immutable and still satisfy the invariants throughout the rest
of the toolchain for recovery tokens. Thanks to chandlerc for suggesting
this approach!
As part of this, move functions that seem reasonable to make out-of-line
to a separate `_impl.h` header file that is only included where the
explicit instantiation _definition_ is provided.
By using explicit instantiation we can make these templates behave more
like non-template classes in terms of supporting out-of-line definitions
that don't need to be compiled by every translation unit. The set of
eventual instantiations here is fundamentally known, and there tend to
be headers that define a canonical "leaf" type where it makes sense to
trigger the explicit instantiation.
Where we already had a `.cpp` file to put the explicit instantiation
definition, use it. But in some places we didn't have such a `.cpp` file
so this PR adds those.
This also requires that we have precise constraints on APIs that _can't_
be instantiated for specific argument types, as now we don't do this
lazily.
Combined, this appears to reduce the sum of object file sizes in the
`check` directory by almost 40% (122mb -> 74mb) in my measurement.
My actual goal was to improve compile times, but so far I don't have a
great methodology for measuring these... But the object file size
reduction seems to confirm this is a net win and likely represents a
non-trivial improvement in compile time.
Assisted-by: Antigravity with Gemini
Mainly because "sorting_diagnostic_consumer" is legacy, since
`SortingDiagnosticConsumer` became `SortingConsumer`. Also better
reflecting contents of these files.
Where I'm not renaming, I'm less positive about dropping "diagnostics"
from "file_diagnostics" and "null_diagnostics" (which contain both a
consumer and emitter, and "null.h" seems like poor naming), so not doing
that here. Also "diagnostic.h" contains `struct Diagnostic`, so is a
decent fit.
Assisted-by: Google Antigravity with Gemini 3 Flash
Trying to figure out an easy way to debug semir in the prelude, #5703
removed an option to set `--exclude-dump-file-prefix` to empty. But,
this is probably an improvement over that flow... With this change, it's
possible to add `//@dump-sem-ir-file` to a specific prelude file, and
its full IR will be printed. Additionally, it becomes an option with the
default `--dump-sem-ir-ranges=only` to add `//@dump-sem-ir-file` and get
the full file's IR.
Adapts `StringLiteral` to lex characters. Adds a `CharLiteral` token,
which contains a `CharLiteralValue` which is a straight unicode code
point (suggested by zygoloid).
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Split out `TokenInfo` to be able to easily write `using ValueType =
TokenInfo;` on `TokenIndex`. Also fixes a small type issue on
`ValueStore` that affected `mapped_iterator` behavior when writing
`old_tokens_it->first < next_offset`.
Moves LineInfo and CommentData out so that they can easily be set as
`ValueType` on the Index types. I've also been thinking about letting
`ValueStore` take `ValueType` as a parameter instead of requiring it to
be inferred this way, but for these it feels more consistent with the
rest of the toolchain to do it this way.
I'm not doing similar with `TokenInfo` just because the recovery token
splicing makes it more difficult to use `ValueStore`.
This consolidates Lex::TokenizedBuffer::DumpSemIRRange and
Parse::TreeAndSubtrees::TokenRange into a single InclusiveTokenRange,
also making the OverlapsWithDumpSemIRRange function take the new struct.
I considered switching to `llvm::iterator_range<Lex::TokenIterator>`,
but we often want to see if the range is size one. Using `TokenIterator`
just looked like it'd add a bunch of offsetting to make it work; I view
that as low-value overhead.
For example:
```
Lex::InclusiveTokenRange token_range = GetSubtreeTokenRange(node_id);
auto begin_loc = tree_->tokens().TokenToDiagnosticLoc(token_range.begin);
if (token_range.begin == token_range.end) {
return begin_loc;
}
auto end_loc = tree_->tokens().TokenToDiagnosticLoc(token_range.end);
```
would become:
```
llvm::iterator_range<Lex::TokenIterator> token_range = GetSubtreeTokenRange(node_id);
auto begin_loc = tree_->tokens().TokenToDiagnosticLoc(*token_range.begin());
if (token_range.begin() + 1 == token_range.end()) {
return begin_loc;
}
auto end_loc = tree_->tokens().TokenToDiagnosticLoc(*(token_range.end() - 1));
```
So I'm keeping the bespoke struct.
This prints instructions that are inside the range, and entities that
overlap with the range. Note this can lead to incomplete printing of
entity contents.
Syntax rationale is on `DumpSemIRRange` to try and record this, since
I'm not sure this belongs in the language design. The intent of this is
to be able to subset SemIR, which will be done separately in the
formatter.
---------
Co-authored-by: David Blaikie <dblaikie@gmail.com>
What this really does is avoids shadowing names, so that we can
comfortable have things like `Check::DiagnosticEmitter` or
`Check::DiagnosticLoc` without shadowing being a concern.
Note, down this path I'm also thinking about:
- Renaming misc DiagnosticConsumer/DiagnosticEmitter classes, possibly
just to DiagnosticConsumer/DiagnosticEmitter (so
`Check::DiagnosticEmitter` instead of `SemIRLocDiagnosticEmitter`).
- Dropping `Diagnostic` from `Emitter::DiagnosticBuilder`.
- But not for `Check::DiagnosticBuilder`, because `Check::Builder` would
be ambiguous.
- Renaming diagnostics/diagnostic_* to drop "diagnostic".
[Discussion about SemIRLoc ->
DiagnosticLoc](https://discord.com/channels/655572317891461132/655578254970716160/1353771570463768698)
reminded me of this (in particular the older [Check::DiagnosticBuilder
discussion](https://discord.com/channels/655572317891461132/655578254970716160/1344363562608627763)),
but I'd only do that rename if there's matching consensus about a path
forward where we keep SemIRLoc, and in a way that it's only ever used
for diagnostics (the divergence from which is at the root of current
LocId discussion).
I'm trying to keep that separate from a namespace addition for clarity.
At present, we typically define a DiagnosticConverter, then store an
instance of it and a DiagnosticEmitter that wraps it. This is relatively
minor in general, but I've been trying to create more self-contained
DiagnosticEmitter classes (which hold their own DiagnosticConverter,
similar to NullDiagnosticEmitter), and there it just gets in the way.
Since we don't reuse DiagnosticConverter instances, this combines the
definition into DiagnosticEmitter. Mainly this means we don't have a
separate object in play, and less to carry around.
The most impact is probably to SemIRDiagnosticConverter, which was also
the most complex. Now `SemIRLocDiagnosticEmitter`, this gets some
different construction flow. Note in the PR I've split the file rename
to its own commit, to try to help delta views. However, the most
substantial parts of the refactoring are split into #4876, which this
depends upon.
I'm looking at eliminating `DiagnosticConverter`. This change removes
`NodeLocConverter` (albeit adding `UnitAndImportsDiagnosticConverter`),
and in doing so, refactors lex conversion functions to extract them out
from the `DiagnosticConverter` functions.
I'll be following up with changes that collapse `DiagnosticConverter`
logic into `DiagnosticEmitter` locations. The intent is that we
shouldn't need separate ownership of both types.
- Escape dumped token strings (what got me here)
- Change the quoting from backticks to quotes
- Also add a `FormatEscaped` helper function for this, updating other
`.write_escaped` uses
This change deliberately breaks away from the line/column ordering, and
instead focuses on a last byte offset corresponding to the final token
processed as part of producing the message. Where that's equal, this
maintains stable ordering in order to reflect the order that diagnostics
were produced.
The intent of this approach is that lex, parse, and check diagnostics
are interleaved based on where they are produced, but that
subexpressions still have diagnostics emitted prior to containing
expressions. In particular, the prior line/column sort essentially
sorted on the _start_ of where a diagnostic was associated, and this is
closer to sorting based on the _end_. As a consequence, something like
`F(1 2)` will have the error for `1 2` emitted _before_ a diagnostic for
`F(1 2)` not matching parameters, instead of _after_.
In check, we track the last handled node. This provides a
last_byte_offset _separate_ from where a diagnostic is associated. The
intent is that this creates an ordering of diagnostics which may be
associated with earlier code, to cause the diagnostics to be emitted
later. An example consequence of this is the change in ordering of
modifier diagnostics: we are diagnosing those from the same place, but
they have the same last_byte_offset, so we print them out in the order
produced.
I've added similar tracking to parse, but cannot identify any test which
is affected by it (note the separate commit, I thought about this late).
I'm not sure whether we have good out-of-order errors we could produce
for this.
A significant number of tests have reordered diagnostics as a
consequence of this change, so this change does not add further testing.
- Provide `Check::Dump(context, arg)` and similar.
- gdb and lldb should do contextual lookup, and `call Dump(*this,
Lex::TokenIndex::Invalid)` has been tested with gdb.
- Since this is only for debug, keeps the functions fully separated from
code.
- Uses alwayslink to ensure objects are correctly linked, even though
there are no calls.
- `-Wno-missing-prototypes` is needed when we don't have forward
declarations.
- Code is not linked in opt builds, using `#ifndef NDEBUG`.
- This probably could be doing something in BUILD files with a
`select()`, but the `#ifndef` seemed easier.
This is based on #4620, but uses free functions instead of member
functions.
Co-authored-by: Dana Jansens <danakj@orodu.net>
---------
Co-authored-by: danakj <danakj@orodu.net>
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.
Previously Collect() was used for types that implemented
CollectMemUsage() but otherwise Add() was used. This required the caller
to think about the type of the field and know/decide which method to
use.
Now, the caller always uses Collect() unless they are adding specific
byte values, in which case Add is used. Typically then, Add will only be
used to implement the CollectMemUsage() function.
To do this we require all Collect() methods to be templates so that they
all be a single overload set. The Collect on BumpPtrAllocator is
converted to a template that checks
`std::same_as<llvm::BumpPtrAllocator, T>`.
This is in anticipation of making the integer value store be customized
heavily. I'd like to extract it from the common code when doing that, so
first disentangling them here without any intended change in
functionality or behavior to enable that.
I've tried to update `#include`s to be as minimal as I can and added a
few missing includes spotted in the process.
I've split the test for value store to include what was easy focused on
just the value store templates rather than the unified shared value
stores.
This might surface some opportunities for adding more tests, but for
this PR, just doing the minimal restructuring.
Trying to make split file tests of lex functionality shorter and easier
to read. numeric_literals.carbon in particular has an example of why I'm
interested in this (at the bottom). This also switches from `[]` list
format to `-` list format so that the trailing `]` is removed.
Trimming comments in tokenized_buffer.h because (1) it feels like it's
giving too much detail about what's printed, which has drifted slightly
and (2) it also feels like it's trying to justify YAML output, when
that's just what we're doing in general.
---------
Co-authored-by: Geoff Romer <gromer@google.com>
This is improving the comment production to produce fewer distinct
comments.
At present, comment processing uses strict prefix matching. It either
expects `// ` (with a space) for valid comments, or just `//` (without a
space) for invalid comments that lacked the space.
As a consequence, the following would be three comments:
```
// Comment 1
//
//
// Comment 4
```
This is because a 3-character prefix is used for valid comments. The
prefix switches between lines 1 and 2, and again between lines 3 and 4,
each resulting in a separate comment.
For contrast, this is one comment because only a 2-character prefix is
used:
```
//Comment 1
//
//
//Comment 4
```
That's because all lines lack a suffix space.
Additionally, with SIMD 16-byte boundaries, further splits can occur if
processing needs to transition to non-SIMD.
Here, I'm trying to just address all of this by:
1. Stitching together adjacent comments. Since a lexed comment starts at
the `//` excluding the indent, the delta from the prior comment must be
precisely the indent.
2. Adding support for switching from SIMD to non-SIMD on file
boundaries.
I considered trying to have a separate `//\n` prefix for SIMD processing
of `// `, but I wasn't sure about the tradeoff of doing both at the same
time (in particular, it'd require constructing a string for the
different prefix), thus this stitch approach. This does mean multiple
passes will be required for a typical long comment structure using blank
comment lines to separate paragraphs (for performance reasons, I will
recommend engineers not write comm... nevermind).
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
In order to format comments, it's helpful if they're tracked. This
tracks them separately from tokens in order to avoid interfering with
parse; it'd be inconvenient if comment tokens could show up in arbitrary
locations, albeit possible to support.
This additionally extracts out the TokenIterator support into a template
in order to generally have it available for IndexBase types. I'm only
adding it for CommentInfo, not sure if we'll want it elsewhere, but this
structure still felt like a good fit.
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>
This makes each token info consist of 8 bytes of data:
- 1 byte of the kind
- 1 bit for whitespace tracking
- 23 bits of payload
- 32 bits for byte offset in the file
This builds directly on representing the location of the token as
a single 32-bit offset, now compressing the rest of the data into
a single 32-bit bitfield.
This adds some implementation limits: we can no longer lex more than
2^23 tokens in a single source file. Nor can we have more than 2^23
string literals, integer literals, real literals, or identifiers. Only
the first of these is even close to an issue, and even then seems
unlikely to ever be a problem in practice.
The memory efficiency here is great and the motivating goal. But to make
this work well, we also need to streamline how we create the tokens.
Otherwise, all the bit fiddling can end up erasing our gains. This PR
adds a number of APIs to manage creating and accessing the now
significantly more complex storage of token infos to try and help with
this.
One big change required to simplify the writes here is to switch from
computing whether a token has trailing space after-the-fact to
pre-computing whether a token will have leading space. That lets us have
the leading space information available immediately when forming the
token, and avoids doing a single bit flip afterward.
Another change that helps with this representation is to minimize the
updating of groups after-the-fact. The code now tries to set the opening
index directly when creating the closing token and only updates the
opening group afterward. Because of the bit packing, this is a reduction
of 0.5% of dynamic instructions in the compile benchmark, and has
dramatic improvements for the grouping symbol focused benchmarks.
All combined, this is a significant improvement on the lexer-focused
benchmarks despite the added complexity, and a significant win on our
compile time benchmarks due to both the lexer improvements and
downstream memory density improvements: 5-12% reduction in lex time,
growing larger as files get larger. About a 4.5% reduction in parse
time, and even a 1-2% reduction in total check time. =D
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Geoff Romer <gromer@google.com>
First, this replaces the separate line index and column index in the
token information with a single 32-bit byte offset of the token. This is
then used to compute line and column numbers with a binary search of the
line structure and then using that to compute the column within the
line. In practice, this is _much_ more efficient:
- Smaller token data structure. This will hopefully combine with a
subsequent optimization PR that shrinks the token data structure still
further.
- Fewer stores to form each token's information in the tight hot loop of
the lexer.
- Less state to maintain while lexing, fewer computations while lexing.
We only have to search to build the line and column information off the
hot lexing path, and so this ends up being a significant win and shrinks
some of the more significant data structures.
Second, this shrinks the line start to a 32-bit integer and removes the
line length. Our source buffer already ensures we only have 2 GiB of
source with a nice diagnostic. I've just added a check to help document
this in the lexer. The line length can be avoided in all of the cases it
was being used, largely by looking at the next line's start and working
from there. This also precipitated cleaning up some code that dated from
when lines were only built during lexing rather than being pre-built,
which resulted in nice simplifications.
With this PR, I think it makes sense to re-name a bunch of methods on
`TokenizedBuffer`, but to an extent that was already needed as these
methods somewhat predate the more pervasive style conventions. I avoided
that here to keep this PR focused on the implementation change, I'll
create a subsequent PR to update the API to both better nomenclature and
remove deviations from our conventions.
There may also be a way to de-duplicate the binary search in the
diagnostic location conversion and the main line accessor binary search,
but it wasn't obvious to me that it would be a net savings, so left it
alone for now.
The performance impact of this varies quite a bit...
The lexer's benchmark improves pretty consistent across the board on
both x86 and Arm. For x86, where I have nice comparison tools, it
appears 3% to 20% faster depending on the specific pattern. For Arm
server CPUs at least it seems a much smaller but still an improvement.
The overall compilation benchmarks however don't improve much with these
changes alone on x86. Significant reduction in instruction count
required for lexing, but the overall performance is bottlenecked
elsewhere in the overall compilation it seems. However, on Arm, despite
the more modest gains in special cases of lexing, this shows fairly
consistent 1-2% improvements in overall lexing performance on our
compilation benchmark. And the expectaiton is these improvements will
compound with subsequent work to further compact our representation.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
`i32` is retained as a special case for now, for bootstrapping purposes,
and maps to `BuiltinIntType`, which is distinct from `Core.Int(32)`.
This will be removed later once we support `Core.BigInt`.
For now this provides both the `iN` types and also the builtins to
support `Core.Int(N)`. The intent is that we'll change the `iN` support
to rewrite to calls here when we do that for the other type literals and
type keywords.
No conversions between integer types are supported yet, and all literals
are of type `i32`, so we can't actually form values of any of these new
types.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Carbon Infra Bot <carbon-external-infra@google.com>
As discussed around #3792, identify the import a diagnostic message came
from prior to the diagnostic message itself. This occurs during location
translation so that the logic can be central.
I'd considered associating the parse node with ImportRef instructions,
but I realized about halfway through that because I need to store the
ImportDirectiveId on the ImportIR for cross-package imports, it's there
for use in location translation without extra work. That saves a fair
amount of stringing it through declarations, as well as an oddity where
ImportRef instructions would have a node that didn't really represent
them.
Since the addition of TranslateArg, I don't think this type is going to
go away (cutting a TODO). Refactoring names slightly to fit the current
role, and adding const to ConvertLocation.
I wanted to choose one or the other. I think some code has been using
each from early on. We're predominately using `filename`, so
consolidating on that. This conveniently matches the [Google dev doc
style guide](https://developers.google.com/style/word-list#filename)
(which we use for docs) which says "filename: Not file name".
In toolchain:
```
╚╡git grep file_name . | wc -l
35
╚╡git grep filename . | wc -l
489
```
Move handling of mismatched brackets out of the main lexing loop into a
separate pass that is only run if there are mismatched brackets This is
done in preparation for using both lookahead and lookbehind to work out
how to match brackets, and to get this code far away from the hot lexing
loop.
Fix bracket insertion location to be immediately after the token that
we're inserting the bracket after, rather than potentially at the end of
a comment. When there are open brackets at the end of the file, say that
there are open brackets, not that there's a closing bracket without a
matching opening bracket.
We have `StringLiteral`s in multiple other `Carbon` sub-namespaces.
Rename to a more specific name to avoid collisions.
We should likely also rename `Carbon::IntId` -> `Carbon::IntValueId` and
`Carbon::RealId` -> `Carbon::RealValueId`, but this collision is
prioritized because it was blocking work on typed parse nodes which
introduces a `Carbon::Parse::StringLiteralId`.
I was suggesting this because `FloatingPoint` is pretty long. `int` and
`float` should be familiar abbreviations. `unsigned` should be familiar
to developers too, but `UnsignedInt` still feels usefully clearer for
the additional chars.
This is an incremental improvement on our diagnostic messages that
simply underlines an entire token if the token is larger than 1 char
(else it points to the single char with a caret like it used to).
Following up on discussion yesterday regarding this split.
Note, I'm expecting #3341 to do IdentifierId -> NameId in SemIR. It
might be worth adding NameId creation directly to StringStore if you're
content with this setup though.
I'm looking at this due to the conversation on #3341. Although
diagnostics aren't where they should be, I thought it may help to start
adding raw identifier support (which may also help show how I was
thinking about this).
Note regarding the TODO on how to form the token, `GetTokenText` returns
the `string_id`'s reference value for an `Identifier`. So to make
`GetTokenText` work in a way that returns `r#foo` for a raw identifier,
I think there are a few options:
1. Add additional data indicating the end of the identifier.
2. Add `RawIdentifier` as a token kind to indicate that it's raw and
should be prefixed with `r#` (but also giving later stages one more
token kind to handle)
3. Make the `string_id` correspond to `r#foo`, and have later stages add
`foo` to the strings table whenever `r#foo` is encountered (with map
lookups leading to deduplication).
4. Add `StringId::RawKeyword` special values for each keyword.
- This would mean `self` prints as `self`, `r#self` prints as `r#self`,
but `r#foo` is not a keyword so prints as `foo`.
- This means keywords would need to be listed in a place `StringId` can
depend on them, one way or the other (e.g., a `keywords.def` file in
`base/` should work).
5. Say that it _is_ an `Identifier`, and if it's a keyword spelling, it
must have been a raw identifier.
- Same limitation as above: This would mean `self` prints as `self`,
`r#self` prints as `r#self`, but `r#foo` is not a keyword so prints as
`foo`.
I'm hoping to resolve this issue separately though. :)
Building on #3311, which started moving the result string into a
`unique_ptr`, instead have `StringLiteral` use a `BumpPtrAllocator` to
manage memory. But also, detect when a string is really trivial during
`Lex` and, if so, return `contents_` directly.
I spent (a lot) of time working to see if there was any profitable way
to port the SIMD code that scans for identifier length to Arm. There
isn't really. =/ While working on these, I made some cleanups to the
SIMD code that seemed worth landing, and added some benchmarks. All this
PR does is the cleanups, benchmarks, and documents that Arm isn't just
waiting to get attention but doesn't really have good options (so far).
For posterity, here are the core techniques I tried:
1) Direct 32-byte SIMD scanning using pair-wise add trees to build
a 32-bit mask of valid identifier and then `clz` to compute the
distance. This is a very good analog to the 16-byte SIMD structure
used on x86-64. The pair-wise summing technique is the one used in
simdjson for similar purposes.
2) A 16-byte SIMD scanning similar to the x86 version but using `shrn`
to produce a 64-bit scalar bitmask with 4 bits per byte, and then
scaling the bit-count distance.
3) Various hybrid versions of (1) and (2) with short scalar scans to
identify short identifiers before paying the SIMD start-up cost.
4) A much fancier version of (1) that scanned 64-bytes at a time, but
cached the resulting 64-bit mask and re-used it until exhausted.
Some good background on these techniques on Arm CPUs is in this blog
post:
https://community.arm.com/arm-community-blogs/b/infrastructure-solutions-blog/posts/porting-x86-vector-bitmask-optimizations-to-arm-neon
Sadly, both (1) and (2) were significantly slower than a scalar loop
over the bytes. Even (3) was consistently slower.
The only approach that came close was (4) and it was very *slightly*
slower in typical examples and very *slightly* faster in extremely
difficult cases like huge identifiers.
Ultimately, the only path I see (suggested by Dougall on a Mastodon
discussion of this whole problem space) is to take (4) to the limit of
computing an identifier-or-not bitmask *for the entire source file*
using a deeply throughput optimized routine (maybe as part of the line
scanning). That should be able to manage the high latency you end up
with when handling these patterns in SIMD on Arm.
The good news is that at least the M1 is *so* fast in the byte-scanning
loop that this isn't hurting nearly as much as I feared.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Richard Smith
Chandler Carruth
Jon Ross-Perkins
Thomas Köppe
Dana Jansens
cui fliter
Jacob Schneider