This updates SourceBuffer to diagnostics. Some additional edits to
diagnostics were necessary due to issues moving arguments around, which
seems to stem from a compile error with clang 14 (fixed in later
versions).
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
### Description:
This pull request introduces two significant changes:
**Improved Error Handling in `remap_file` Function:**
- The error handling in the `remap_file` function has been simplified
for improved clarity and user-friendliness. The original code included a
combination of `assert` and `exit`, which could be perplexing during
runtime.
**Changes Made:**
- Replaced the original `assert` and `exit` with a more straightforward
`if-else` block for handling errors.
- Utilized `sys.exit` for clear error messages and proper program
termination when an error occurs.
**Why I Did It:**
- Improved Clarity: The original code's `assert` primarily served
debugging purposes and might not behave as expected during runtime. The
new code offers clarity and predictability.
- User-Friendly Errors: The updated code provides users with easily
understandable error messages.
**Added `.DS_Store` to `.gitignore`:**
- `.DS_Store` files are commonly generated by macOS Finder to store
folder-specific metadata. They do not need to be tracked in version
control.
**Changes Made:**
- Added `.DS_Store` to the `.gitignore` file.
**Why I Did It:**
- To prevent `.DS_Store` files from being tracked in the Git repository,
ensuring a cleaner repository and avoiding unintended commits of
macOS-specific files.
### Improvements:
- **Better Error Handling:** The new code in the `remap_file` function
ensures the program exits gracefully with informative error messages
when errors occur.
- **Enhanced Code Clarity:** The code now communicates its intentions
more clearly with `sys.exit` for handling expected runtime errors.
- **Cleaner Git Repository:** The addition of `.DS_Store` to
`.gitignore` prevents the tracking of macOS-specific files in version
control.
Rearranges driver logic into CompilationUnits in order to associate
artifacts from the various stages of compilation.
Note, I'm not totally sure what the right thing to do is for
lower/codegen, so I'm just doing a rote change there for now that
mirrors prior phases (this is all the code supports anyways, so is
probably right for now regardless).
SourceBuffer error output is moved local for consistency with other
steps, and so that it's less ambiguous whether the error should be
expected to already include a filename.
In general, LLVM's parsing of decimal integers to `APInt`s forms an
`APInt` that is 4n bits wide, where n is the length of the integer, and
the `isNegative` check only checks the high bit.
In this case, we form an `APInt` that is four bits wide, with the high
bit set, which we reject because we think it's "negative". These two
array lengths are the only ones where this happens -- if the decimal
integer value is two characters long, we form an `APInt` that is eight
bits wide but holds a value < 100, so the high bit is never set, and the
same applies for longer integers too.
An `IntegerLiteral` is never negative, so we don't need the `isNegative`
check, and in fact it only detects the n=8 and n=9 cases.
IntegerLiterals are not signed, so get the zero-extended value rather
than the sign-extended value. Sometimes we use the high bit of the
`APInt`, though currently this only happens for the literals 8 and 9 due
to the way we convert decimal integers to binary.
The expensive local actions will be separately gated to not overwhelm
the machine, and currently highly asynchronous actions are a dominant
part of our builds due to downloading cached artifacts. Without a high
concurrency, these are downloaded roughly 2-at-a-time currently.
I've verified that on a small machine without a good cache this doesn't
seem to generate huge amounts of work and local build and test actions
are successfully gated on the local flags.
There is already a Bazel issue tracking this limitation:
https://github.com/bazelbuild/bazel/issues/6394
Continuing with #3070. Just a dir and file rename (only prefix change is
lexer_file_test). Everything in the lex dir should be marked as a move.
Note, I think this closes#3070. There may still be further cleanup
later, but the organizational changes suggested there are being
completed.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Continuing with #3070. Just a dir and file rename (mostly removing
prefixes, although for parse_tree_fuzzer and parse_tree_file_test I'm
dropping "tree" instead of "parse"). Everything in the parse dir should
be marked as a move.
Continuing along with #3070. Note this is just a file rename, with BUILD
edits; every file previously in semantics/ should show as moved (except
maybe BUILDs, which split).
Versus the namespace change in #3170, these felt like they may get more
nuanced review, so I'm splitting them out:
- Removing "Lexed" from LexedNumericLiteral and LexedStringLiteral
- Moving classes out of the TokenizedBuffer class, so that they're now
things like Lex::Token instead of Lex::TokenizedBuffer::Token (i.e.,
much shorter to type, more consistent with things like Parse::Node).
Continuing with #3070, this creates a `Lex` namespace for lexer. This is
probably the last namespace addition right now, and will be followed by
file moves, although I'll also share a PR separately moving member
classes out of TokenizedBuffer.
This is necessary for a clean split of the SemIR and Check namespaces.
My design intent with check.h is that check/check.h provides the factory
functions necessary to construct a SemIR, which is the main reason I
have the MakeBuiltins wrapper there.
I don't think File's constructors are great, but it felt good enough to
me in that context. I considered factory functions, or something like an
enum as discriminator, but it felt like more burden than improvement.
Specify the behavior of function calls and the type and behavior of the
entity
introduced by a function declaration.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Describe the broad expression category (value / reference /
initializing) in the semantics IR textual format. This is redundant
information that can be calculated from the text that's already in the
output, but seems useful for understanding the structure of the IR.
Switch from a fully lowering-oriented approach to initializing
expressions in semantics IR to an approach that retains more information
about the high-level semantics of the program. Specifically:
- Always form an `Assign` node for a variable initialization or
assignment, even for empty types and when the actual initialization is
performed in-place by evaluating the right-hand side.
- Always form a `ReturnExpression` node for a return statement with an
expression, even when the actual initialization is performed in-place
into the return slot and the function doesn't return a value.
This is intended to permit us to more easily write semantic checks over
the IR, and to preserve a use edge from the use of an initializing
expression and its initializer in all cases.
To better support this, `MaterializeTemporary` is also split into two
nodes:
- A `TemporaryStorage` node allocates and provides the storage for a
temporary, and is emitted prior to emitting the initialization of the
temporary.
- A `Temporary` node connects the temporary storage to the initializer,
and is emitted after emitting the initialization.
Example testcase:
```carbon
fn F() -> (i32, i32);
fn G() -> (i32, i32) {
var v: (i32, i32) = F();
v = F();
return F();
}
fn H() -> i32 {
return G()[0];
}
```
Before:
```carbon-semir
fn @G() -> %return: (i32, i32) {
// ...
%v: (i32, i32) = var "v"
%.loc4_24: (i32, i32) = call @F() to %v
%.loc5: (i32, i32) = call @F() to %v
%.loc6: (i32, i32) = call @F() to %return
return
}
fn @H() -> i32 {
!entry:
%.loc10_11.1: (i32, i32) = materialize_temporary
%.loc10_11.2: (i32, i32) = call @G() to %.loc10_11.1
%.loc10_14: i32 = int_literal 0
%.loc10_15.1: i32 = tuple_index %.loc10_11.1, %.loc10_14
%.loc10_15.2: i32 = bind_value %.loc10_15.1
return %.loc10_15.2
}
```
After:
```carbon-semir
fn @G() -> %return: (i32, i32) {
// ...
%v: (i32, i32) = var "v"
%.loc4_24: (i32, i32) = call @F() to %v
assign %v, %.loc4_24
%.loc5: (i32, i32) = call @F() to %v
assign %v, %.loc5
%.loc6: (i32, i32) = call @F() to %return
return %.loc6
}
fn @H() -> i32 {
!entry:
%.loc10_11.1: (i32, i32) = temporary_storage
%.loc10_11.2: (i32, i32) = call @G() to %.loc10_11.1
%.loc10_14: i32 = int_literal 0
%.loc10_11.3: (i32, i32) = temporary %.loc10_11.1, %.loc10_11.2
%.loc10_15.1: i32 = tuple_index %.loc10_11.3, %.loc10_14
%.loc10_15.2: i32 = bind_value %.loc10_15.1
return %.loc10_15.2
}
```
Note that in the "Before" code, there is no indication in `G()` that we
assigned to `v`, and no path through "use" edges from the tuple indexing
in `H()` to the call to `G()`.
This change results in our not performing `memcpy`s into the destination
for tuple and struct initializers. This is a consequence of those
initializers not yet performing in-place initialization, and will be
fixed in a subsequent change.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
With the toolchain splitting namespaces, ostream.h's `operator<<`
templates aren't reliably found with name lookup, likely due to the loss
of associated namespaces (zygoloid commented on this at
https://github.com/carbon-language/carbon-lang/pull/3161#discussion_r1307941999).
This is especially a barrier to moving the lex files into `Carbon::Lex`;
versus other parts of the toolchain, they contain more printable types
which are used cross-namespace, including `Carbon::Testing`. As a
consequence, I'm looking at migrating ostream.h to a more reliable
approach that doesn't rely as much on everything being in the `Carbon`
namespace.
Change terminology away from terms that are ambiguous:
- Reserve "generic type" for types with (compile-time) parameters, like
`Vector` in `Vector(T:! type)`. Don't use that term to refer to `T`, as
it would with
[#2360](https://github.com/carbon-language/carbon-lang/blob/trunk/proposals/p2360.md#terminology).
- Use the term "compile-time" instead of "constant" to mean "template or
symbolic." Expand the term "constant" to include values, such as from
`let` bindings.
This function hid the `Value::Print` function from the base class, and
provided different output. As far as I can tell, the only caller is the
implementation of `Value::Print`.
Refactors `CallDestructor` and `CallFunction` to both call a new method
`BindSelfIfPresent`, which includes support for binding `addr self` if
specified. Fixes the associated unit test.
Closes#2802.
This fixes an issue with `constexpr` in ARM builds. The table needs to
be a _`static`_ `constexpr` in order to be used w/o capture in the
lambda. This was reported with an alternative fix in #3164 -- this fix
avoids adding captures to the lambda by fixing the `constexpr`
declaration.
The name is tweaked and parentheses added to try to keep `clang-format`
producing a nice formatting for this weird construct. Without these, I
was getting distractingly bad results.
The code path wasn't built outside of ARM, and so mostly showed up on
ARM macOS builds -- in general, we don't currently have non-x86 GitHub
CI to catch this kind of issue. Sorry for folks who bumped into it!
I'm planning a subsequent PR that will refactor code so that we have
more common code in the fallback and reduce our exposure to
single-platform build issues like this.
#2569 added PrintAsID to //common/ostream.h, but given it's
explorer-specific behavior, I don't think it's the right home for it.
Noticed this while pondering better ostream interfaces.
Continuing on #3070.
I moved ParseTree::Node to just Parse::Node, versus Parse::Tree::Node.
Other name changes are just removing "Parse" or "Parser" prefixes.
In EnumBase, I'm directly defining operator<< because the ostream.h
approach just isn't working, not for either of Parse::State nor
Parse::NodeKind. Errors look like:
```toolchain/parser/parser_context.cpp:449:34: error: invalid operands to binary expression ('llvm::raw_ostream' and 'const Carbon::Parse::State')
output << "\t" << i << ".\t" << entry.state;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~
```
The expected template in `Carbon::` is not in the error list; I only see
the:
```
./common/ostream.h:112:6: note: candidate template ignored: requirement 'std::is_base_of_v<std::ostream, llvm::raw_ostream>' was not satisfied [with S = llvm::raw_ostream, T = Carbon::Parse::State]
auto operator<<(S& standard_out, const T& value) -> S& {
^
```
I'm still prodding at this, but not seeing an obvious fix.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
The different approach to Names avoids the issues with trying to define
a static member (or also member function) of the templated instance of
Carbon::Internal::EnumBase from a non-enclosing namespace such as
Carbon::SemIR.
Note, I'm trying to do this from the cpp file. An alternative might be
to do `inline constexpr llvm::StringLiteral Names[]` in the .h file, but
I think concerns had been raised about that needing deduplication.
Splitting namespace changes from file renames with the thought that
it'll be more likely to work with git's merge logic.
Renamed LoweringContext to FileContext to disambiguate more clearly from
FunctionContext.
This is part of #3070
The goal of this proposal is to provide a way for user-defined types to
support range-based iteration with `for`.
The current proposed solution exposes 3 interfaces that can be
implemented by user types to enable support for
ranged-for loops.
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
---------
Co-authored-by: josh11b <josh11b@users.noreply.github.com>
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Co-authored-by: Geoff Romer <gromer@google.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Previously, accessing any property of a token's kind required both
a function call to an out-of-line method and loading the relevant data
out of a table. These method calls were often roughly as expensive as
the load out of the table -- they would push and pop registers, and so
would touch a stack cache line not unlike the table cache line.
We can inline them in a way that carefully leaves the table definitions
in the single TU so we don't get tons of copies of data that have to be
merged by the linker. But the access to the table can be in an inline
function that allows the call overhead to evaporate and these to turn
into just table loads in the callers as well.
While in theory we could rely on forms of LTO to acheive this, it
doesn't seem worth relying on that. This is an easy win with very little
cost in reality.
Several of the lexer benchmarks improve by 1% or 2% from removing the
function call overhead. It's not a huge win, but it's nice.
Even more nice is that the profile is cleaned up significantly, clearly
focusing on the core functions where time is spent in lexing and
parsing.
This also pulls my big-picture benchmark of 10mloc file down to under
4.3s to lex and parse excitingly. Basically, we're above 2.3mloc/s
lexing and parsing. Not *quite* the 10mloc/s that I'm hoping for, but
still progress.
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
A lot of this is more boring "remove unused header", plus some other
minor cleanups. I think the most significant changes were:
- yaml_test_helpers.cpp is doing a switch on an unsigned int, comparing
to enum values.
-
[bugprone-switch-missing-default-case](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/switch-missing-default-case.html)
is unhappy with EnumBase, but correctly identified
yaml_test_helpers.cpp, so I'm opting to address it rather than disabling
it even though it needs NOLINT in several locations as a result, in
addition to what I think are some low-value `default` cases. I'd be fine
going the other way with this too and disabling it globally (I could see
it being noisier in the explorer).
- MarkInitializerFor swaps the argument names between the .h and .cpp. I
think the .cpp had the order as intended.
- There's a new-ish
[performance-enum-size](https://clang.llvm.org/extra/clang-tidy/checks/performance/enum-size.html)
which I'm basically treating as "add int8_t to enums".
My main motivation here is to just clean up as many of these as I can so
that I stop seeing them in vscode.
This is a rough script that uses regexes to do a simple scan of source
code and extract some basic source code statistics. Things like column
width, comment line density, identifier lengths and densities.
After scanning, it prints out both raw stats and in a few cases renders
a quick histogram to the terminal to help visualize a relevant
distribution.
I threw this together pretty quickly, and this is an area of Python I
have very limited familiarity with, so happy to have any suggestions for
how to better approach this.
Trying to improve diagnostic names and code flow. I was thinking
"operand" might be a better name than "name" in the context of what `a`
is in `a[b]`, where we already call `b` the `index`.
Flow-wise, note this replaces pushing BuiltinError with instead creating
Index nodes that may contain an error. This is intended to aim towards
producing a more consistent SemIR in the face of errors. There are pros
and cons of both approaches, but I'm aiming for simpler control flow.
The successor strategy talk isn't up yet, but will be on Sep 8 -- just
trying to get them all in one pass.
Slides were previously linked, which I could keep, although it's weird
to have a long link with just `(Slides)` at the end because it implies
the whole linked text is slides (rather than a video). So if that's
desired, I might switch instead to something like:
```
- Talk name, con ([video](youtube link), [slides](slides link))
```
But, this current format (video-only full-name link) felt consistent
with the 2022 links.
This PR removes `PrintDepth` from statement and declaration.
Implements `PrintIndent` for better indented formatting along with
various changes to make printing of statements and declarations better.
---------
Co-authored-by: Jon Ross-Perkins <jperkins@google.com>
Pass function parameters directly or indirectly, depending on their
value representation. For types such as empty tuples with an empty value
representation, don't pass them at all.
For types using a pointer value representation, emit an `llvm.memcpy`
call to perform assignment rather performing a first-class aggregate
load and store.
It was pointed out in review that the approach to building random inputs
for the benchmark would produce significantly varying input lengths and
cause unavoidable noise of as much as 3% between runs.
While initially, these were only used for very coarse measurements, this
is a definite problem, and so this PR restructures things along the
lines suggested. Rather than generating random lengths or random
selections according to distributions, instead the lengths and coverage
of sets are done deterministically. Then the sequences are randomized in
order to prevent getting stuck in a silent local minima or maxima.
The technique works to ensure that the randomness is done on every run
of the benchmark, not just every run of the program, so that we don't
have noise hidden from run-to-run. That means fresh memory allocation
and some wasted time computed freshly shuffled inputs, but makes sure
the unavoidable noise from things like ASLR show up (as much as
possible) even with simple repetitions of the benchmark runs.
As part of this, this PR moves towards building an entirely custom
distribution of identifier lengths based on the direct measurements of
the LLVM codebase. These measurements are provided by a script that will
be in a subsequent PR, but the exact distribution doesn't matter as much
as our ability to control it and build on it in a fully deterministic
way.
To help with analyzing all of these, we also start tracking the bytes
processed in addition to the tokens processed as rates. The naming is
changed to be similar, and this produces the following nice output from
the benchmark now:
```
---------------------------------------------------------------------------------------------------------------
Benchmark Time CPU Iterations bytes_per_second tokens_per_second
---------------------------------------------------------------------------------------------------------------
BM_ValidKeywords 3365571 ns 3365577 ns 206 177.449M/s 29.7126M/s
BM_ValidIdentifiers<1, 64, false> 12646151 ns 12645834 ns 52 116.466M/s 7.90774M/s
BM_ValidIdentifiers<1, 1, true> 4388390 ns 4388141 ns 161 65.1988M/s 22.7887M/s
BM_ValidIdentifiers<3, 5, true> 16286551 ns 16286572 ns 43 35.1334M/s 6.14003M/s
BM_ValidIdentifiers<3, 16, true> 15798770 ns 15797567 ns 44 69.4229M/s 6.33009M/s
BM_ValidIdentifiers<12, 64, true> 15670499 ns 15670257 ns 38 243.421M/s 6.38152M/s
BM_ValidMix/10/40 7415940 ns 7415564 ns 94 134.257M/s 13.4852M/s
BM_ValidMix/25/30 7349454 ns 7349402 ns 95 121.762M/s 13.6065M/s
BM_ValidMix/50/20 6691223 ns 6690969 ns 105 100.3M/s 14.9455M/s
BM_ValidMix/75/10 5131517 ns 5131263 ns 135 86.4556M/s 19.4884M/s
```
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Start treating function calls as initializing expressions instead of as
value expressions.
This required adding support for expression categories. Value bindings
and temporary materialization conversions are created where necessary to
transition between expression categories. For a function call with a
return slot, we speculatively create a materialized temporary before the
call and either commit to it or replace it with something else later,
once we see how the function call expression is actually used.
This change follows the direction suggested in #3133 for initializing
expressions: depending on the return type of a function, the return
value will either be initialized in-place or returned directly. This is
visible in the semantics IR, which is a little unfortunate but is
probably necessary as this is part of the semantics of the program.
---------
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Jon Ross-PerkinsandRichard Smith
m3ndax
josh11b
Geoff Romer
Chandler Carruth
Lucile Rose Nihlen
Prabhat SachdevaandRichard Smith
