This routine is large enough that it sometimes exceeds the inlining
threshold. However, when out-of-line, it forces `Hasher` to be spilled
to the stack in addition to the register pressure of the out-of-line
call. The performance here is really engineered around it being inlined,
so force the issue with an attribute.
This also requires force inlining of the wrappers that bottom out at
this routine, or we'd just move the problem up one layer without fully
solving it.
I don't have specific numbers for this, but it seems like a generally
good change to ensure consistent performance here.
Assisted-by: Antigravity with Gemini
The rendering we're trying to implement based on
`toolchain/docs/diagnostics_rendering.md` needs some features the
initial terminal API didn't support:
- Heavy stroke weight on lines
- Vertical lines that cross earlier lines without joining them
- Wrapping that keeps quoted code whole
This PR adds support for these and removes the two `buffer.h`
TODOs that described them.
This PR also tries to improve the ASCII fallback quality a bit by using
`^`-lines for "heavy" weight, and using `.` and `'` when drawing tees.
Still not perfect of course, but seems reasonable given the constraints
of ASCII rendering.
Assisted-by: Claude Code
These only show up post-merge as we're only running ASan there.
This disables the recent `[[clang::preserve_none]]` optimization when
ASan is enabled as there is an LLVM bug on AArch64 that causes register
corruption with that calling convention. This is factored into a common
helper library so we can use it in other places as well.
Assisted-by: Antigravity with Gemini
Extract the allocation path (which is inherently cold due to growth).
Making the outlining effective requires both blocking further inlining
and using `preserve_most` so that the existence of the call doesn't
disrupt the register allocation.
We also mark the insertion statements with callbacks as `always_inline`
so it being cold doesn't cause it to be left out-of-line and further
disrupt register allocation. It's a bit strange to force inlining of a
cold-path function, but the register impact of the call extends back
into the hot-path.
Agy's benchmarks (`./scripts/bench_runner.py --runs=12`, `-c opt
--copt=-march=x86-64-v3`, vs. `trunk`):
- `BM_ValidIdentifiers<3, 5, true>`: -6.277% CPU time
- `BM_ValidIdentifiers<3, 16, true>`: -4.525% CPU time
- `BM_CompileApiFileDenseDecls<Lang::Carbon, Phase::Lex>/262144`:
-2.012% CPU time (47.5 ms ± 6.358% -> 46.54 ms ± 1.773%, p=0.0167)
Assisted-by: Antigravity with Gemini
An accidental copy would cause an extra save/restore. This is meant as
an RAII object to denote a save for a given scope. Also disallow move so
we don't need to add branches to the dtor.
Diagnostic rendering was part of the data model: `Loc::FormatLocation`
and
`Loc::FormatSnippet` wrote plain text to a stream, so no capability,
color, or
terminal width could reach them.
A diagnostic is now one message with labels, contexts, and location
information
attached, and `Diagnostics::Renderer` draws it into a
`Terminal::Buffer`. The
design is in `toolchain/docs/diagnostics_rendering.md` (#7668),
authoring in
`toolchain/docs/diagnostics.md`.
Before:
```
example.carbon:6:3: error: 1 argument passed to function expecting 0 arguments
Run0(1);
^~~~~~~
example.carbon:3:1: note: calling function declared here
fn Run0() {}
^~~~~~~~~~~
```
After:
```
error: 1 argument passed to function expecting 0 arguments
╭─┤ example.carbon:6:3
┆
3 │ fn Run0() {}
· ─────┬─────
· ╰──┤ calling function declared here
┆
-> 6 │ Run0(1);
· ───────
╰────────────
```
`--no-diagnostic-snippets` gives a compact form, `example.carbon:6:3-9`.
File
tests use it; the rendering tests under `toolchain/diagnostics/testdata`
ask for
snippets.
C++ interop diagnostics are data now rather than text Clang drew:
`SemIR::ClangSourceLocId` names a `clang::SourceRange`, and a Clang
location is
marked as the point it names.
`StreamConsumer` moves to a `:renderer` target that `:emitter` does not
depend
on, keeping `common/terminal` out of `consumer.h`. Capabilities come
from the
file behind the error stream, so tests get plain ASCII.
`renderer_fuzzer` draws diagnostics built from fuzzer bytes with every
capability combination, since a shape the renderer doesn't expect must
never be
why the compiler dies while reporting a problem.
TODOs mark the follow-ups: line weights and crossing lines, wrapping
that keeps
a quoted type whole, and ordering diagnostics that share a byte offset.
Also: `IncorrectExtension`'s hint moves into its message, since its note
had no
location of its own, and `strip_noise` in `check_diagnostics.py` blanks
multi-line `#define`s instead of deleting them.
Assisted-by: Claude Code
When identifying a facet type, defer evaluation of LookupImplWitnesses
by redirecting them to evaluate into IdentifiedWitness. Then the caller
of identify will need to substitute the IdentifiedWitnesses, either with
witnesses of their own, or with LookupImplWitness to cause a lookup
operation.
This allows:
- An `impl as` on a named constraint can subst the witnesses in
designators from the named constraint with a witness to the impl itself.
This prevents them from looking for and finding some other overlapping
impl. (This PR)
- Impl lookup to collect rewrite/equality requirements from all named
constraints and then use the impls it has already found to evaluate any
designators in those requirements instead of performing impl lookup
again (creating a cycle). (Done in
https://github.com/carbon-language/carbon-lang/pull/7944)
- Similarly, an `impl as` can collect impls and equality requirements
from all named constraints and verify them correctly by replacing
witnesses that should refer to the impl with the correct witness instead
of performing a lookup and finding an overlapping impl. (Done in
https://github.com/carbon-language/carbon-lang/pull/7937)
- ImplWitnessAccess to be able to find rewrites from a named constraint,
not just from the initial DeclaredFacetType. (TBD)
To do this we need to introduce the concept of "identified" specifics.
In order to identify a facet type, we must construct a specific for each
require decl in a named constraint. These specifics will have
IdentifiedWitnesses inside them, but their args would otherwise match a
specific for the require decl constructed in another context. To avoid
polluting the canonical specific namespace, we introduce a second
namespace of specifics that are "identified" since they are created by
identifying.
As we have introduced a new global state that modifies eval behaviour,
we must disable that state when resolving a specific (as per
https://github.com/carbon-language/carbon-lang/pull/7923). But inside an
"identified" specific, we preserve the behaviour so that the witnesses
modified by identify (through SubstPeriodSelf or through evaluating a
require specific) have their evaluation deferred.
Sets a variable to a new value, saving a copy of that new value. Then
CHECKs that the variable is still set to that new value before restoring
the original value over top of it. This is meant to catch data loss
where the variable holds some change that would be overwritten by the
restore. In particular, this would catch bad bracketing of sets and
restores.
Hashtables seed every hash with the address of a global,
`ComputeSeed()`, so that ASLR varies layout and iteration order between
runs. Debug builds already vary iteration order directly in `entries()`,
so the seed only matters in release builds. There it costs a register
and an XOR on the latency path of every integer or pointer hash, and it
redraws a table's collisions in each process.
Remove the seed from `HashValue`, `Hasher`, `CarbonHashValue`,
`CustomHashValue::Hash`, and `HashKey`. Hashing starts from a zero
buffer, so integer keys hash as `WeakMix(key)`. `global_addr_seed`
remains in debug builds to seed the iteration-order entropy.
`ClangDeclSignature` chained its hash through the seed, and now hashes
its fields with one `Hasher`.
The seed also kept some short-input paths from collapsing. Without it,
every `{i, 0, 0}` array of `int32_t` hashed the same, all-zero 4-8 byte
inputs lost their size, and the 17-32 byte path multiplied the size
sample by a byte-shifted copy of itself. Each `Mix` operand of the
short-input paths now XORs in a 32-bit constant from
`ImmediateRandomData` in place of a 64-bit chunk of `StaticRandomData`,
which they now only sample to encode sizes. `XorImmediate` keeps these
as `xor $imm32` on x86-64; without its `asm` barrier, a read feeding the
XOR is folded in instead and the constant ends up in a register that
loops hoist.
Cycles against trunk on Zen 5, from `bench_runner.py` with 48 runs per
side, each run at a random code layout, and table sizes of 64, 3584, and
57344:
- Integer and pointer hits: -3.0% to -5.7% in `Map`, -0.8% to -2.4% in
`Set`.
- Integer and pointer misses: up to -12.6%, except `Set<int*>` at +1.4%
(64) and +6.1% (3584).
- String keys: -2.8% to +2.1%, with `Map` updates at +1.1% to +1.6%.
- Hash latency, without layout randomization: integers and pointers
-9.8%, empty strings +5.4% (0.1 cycles), other strings not significant.
Same method on an Apple M1, with #7870 on both sides. Without it, this
change moves benchmark loop counters between `sp`- and `x29`-relative
stack slots, which alone makes `Set<int>` lookups up to 49% slower on
the M1.
- Integer and pointer hits: -1.0% to -10.3% in `Map`, -24.5% to +1.2% in
`Set`.
- Integer and pointer misses: -3.9% to -48.0%, except +7.2% to +59.3% at
3584, from longer probes (instructions +6.4% to +70.6%).
- Inserting 64 integer keys: +12.1% to +15.0%, from store memory-order
violations.
- String keys: -6.6% to +3.9%.
- Hash latency: integers and pointers -9.0% to -10.0%, empty strings
+48.8% (1 cycle).
Release builds now iterate a given table in the same order in every run.
Code that depends on that order still fails in debug builds.
Assisted-by: Claude Code
Clang implements the `"+r,m"` constraint in `benchmark::DoNotOptimize`
by always choosing memory, so each call stores the value to the stack
and loads it back. Most of our benchmarks call it on a loop counter or
another value carried to the next iteration, which puts that store and
reload on the loop's critical path.
On an Apple M1, the cost of that round trip depends on which register
the compiler uses to address the stack slot, and unrelated code changes
move it. Changing only that register from `x29` to `sp`, with the same
address, made `BM_SetLookupHitPtr<Set<int>>` 41.5% to 49.1% slower.
Add `Carbon::Testing::DoNotOptimize` in
`//testing/base:benchmark_helpers`, and switch every benchmark to it. It
only accepts types it can keep in registers: integers, enums, and
pointers go into a `"+r"` constraint with a `"memory"` clobber, and
containers pass their `data()` pointer to that form, so the compiler
must assume the call reads and writes their contents. Any other type is
a compile error. The container form doesn't block optimizing based on a
container's size; passing the container's address does.
Benchmarks that pass a loop counter or carried value no longer measure a
store and reload per iteration, so their numbers aren't comparable with
earlier runs. For example, the hashing latency benchmarks no longer
include one in each hash's latency.
Assisted-by: Claude Code
On Zen 5, a hashtable benchmark's result depends on where its timed loop
lands in memory. With a fixed hash seed, `BM_MapContainsMiss<Map<int,
int>>/64` takes 5.5-6.3 cycles per lookup at 8 of the 16 possible loop
positions within 256 bytes, and 6.7-7.1 at the other 8, through effects
in the op cache, the BTB, and the branch predictor. Which position a
build gets is an accident of the surrounding code, so comparing two
builds conflates a change's effect with where its code landed.
Each timed loop now runs from one of 16 out-of-line copies, aligned to
256 bytes and padded in 16-byte steps, which place the loop at each of
those 16 positions. A process picks a copy at random, or the one named
in `CARBON_BENCH_LAYOUT`, and reports it in the `Layout` counter.
Running `bench_runner.py` with enough runs to sample every position, 48
for example, makes its medians and confidence intervals account for
layout.
In a sweep over 1024 bytes, performance depended only on the loop's
address modulo 256, and loop heads are 16-byte aligned, so finer or
wider padding found nothing more on this CPU.
`CARBON_BENCH_LAYOUT_PAD_STEP` and `CARBON_BENCH_LAYOUT_PAD_RANGE`
adjust it. The copies grow `map_benchmark` from 4.8 MB to 7.6 MB.
On AArch64, LLVM doesn't align loop heads for generic or Apple CPUs, so
they land on any 4-byte boundary and 16-byte steps only reach a quarter
of the positions. AArch64 builds pad in 4-byte steps instead, giving 64
layouts and growing `map_benchmark` there to 16.6 MB. Every copy,
including the unpadded one, also emits the padding `asm` statement,
because its presence changes how the surrounding code is compiled.
Assisted-by: Claude Code
---------
Co-authored-by: Nicholas Bishop <nbishop@nbishop.net>
The hashing code is generic over the type of the value being given to
sizeof() so ideally this warning would not happen at all, but it does.
Possibly because the value is the return of an overload set, so it's not
obvious that it's the templated type. One of those overloads returns
`const void*` but change that to an integer does not remove the warning
still.
clang-tidy 24 warns on C-style variadics, and we don't need to use one
here anymore. Instead of a function call with an argument list, use a
concept to determine if a type can be list initialized.
- Move formatted printing to `hashing.cpp` instead of `hashing.h`
- Separate APInt and APFloat hashing specializations into a new
`hashing_llvm.h`
- Update toolchain/base dependencies and include sites that hash LLVM
data types to include `hashing_llvm.h`
Combined, this reduces the transitive includes caused by `hashing.h`.
Assisted-by: Antigravity with Gemini
`-Wunused-template` was added to `-Wunused`, so clean up the things it
found. One of them was a bug in the Clang warning that I've worked
around and reported upstream:
https://github.com/llvm/llvm-project/issues/218429
Everything drawn into a buffer was checked against `columns()`. That is
right for wrapping and for line drawing, both of which have somewhere
else to put what doesn't fit, but wrong for `DrawText`, which exists for
text that must not be broken and sometimes has to run past the width
with no other answer available. It and `DrawCodePoint` now check only
that the column is non-negative and the row is one a grid can index, and
widen the buffer as far as the text needs; `DrawWrappedText`,
`DrawHorizontalLine`, `DrawVerticalLine`, and `DrawBox` are unchanged.
That also settles what a caller does after a drawing overhangs, since
`DrawEnd` exists so that a run can continue where the last one ended,
and that continuation was itself a checked error whenever the previous
run overhung. A column computed to be negative, such as a gutter
narrower than the line number it holds, still fails.
A color picked to read against black is hard to read against white, and
nothing in `Capabilities` said which a stream was going into.
`ChooseBackground` reads `COLORFGBG`, which `rxvt` and its derivatives
set to the foreground and background palette indices, and takes anything
it doesn't answer to be dark: guessing dark costs contrast, while
guessing light puts pale text on a pale background. Asking the terminal
itself with an `OSC 11` query is the accurate answer, and needs raw
mode, a timeout, and somewhere to put the reply, so there is a TODO for
it rather than an implementation.
Every corner, tee, and crossing came out of `Charset::Ascii` as `+`,
which left six of the shapes a diagnostic draws indistinguishable: the
rule closing a frame read as the one separating two snippets, and the
anchor opening a diagnostic as the one carrying it on. Each stand-in now
keeps the axis its line runs through, which leaves `+` meaning a
crossing and nothing else. A tee keeps its through-stroke and leaves the
branch to what is drawn beside it, and a corner is `.` where its line
leaves downward and `'` where it arrives from above, which is where
those characters sit in their cells. A box is a box again:
```
+--+ .--.
| | -> | |
+--+ '--'
```
Assisted-by: Claude Code
Rich diagnostic rendering needs a layer underneath it that knows what
the attached terminal can do and can position styled text in two
dimensions. This adds that layer, both as the foundation the diagnostics
rendering work will build on and as something usable directly for
ordinary CLI output. Nothing depends on it yet, so it lands and is
reviewed on its own.
Four libraries, each with its own tests:
- `color`: a color, either one of the 16 named ANSI colors or a 24-bit
RGB value, and the escape sequences that select it at a given color
depth.
- `style`: colors plus text attributes, and the escapes that move a
terminal from one style to another.
- `capabilities`: what the terminal behind a stream supports, detected
from the environment.
- `buffer`: a grid of styled cells that layout code draws into and that
renders itself once.
Rationale for the design decisions lives in the headers, next to what it
explains. Four things are worth review attention in particular:
- The color detection precedence documented on `ChooseColorMode`. It
settles how a `--color` flag, `NO_COLOR`, `CLICOLOR`, `FORCE_COLOR`, and
the terminal itself interact. The policy is a pure function of those
inputs, so the whole table is tested without touching the process
environment.
- `Charset`, which decides whether any UTF-8 processing happens at all.
Column counts only follow from code points if the terminal agrees about
the encoding, so anything short of a locale naming UTF-8 is treated as
bytes.
- `Buffer` owning column accounting instead of its callers, which is
what keeps double-width characters, combining marks, and stray bytes
from misaligning everything after them.
- The API surface, which is held to operations that nothing else covers.
Junctions in line art come only from lines overlapping, and turning a
style on or off is spelled as a transition to or from the default style.
`terminal_benchmark` covers style transitions, full-screen rendering,
and text drawing. On an M-series laptop, rendering an 80x24 screen in
which every cell changes style costs about 14us with color off and
76-95us with it, and drawing a 40-column line of source costs about
140ns without UTF-8 processing and 394ns with it.
Assisted-by: Gemini and Claude
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
A defaulted `operator==` or `operator<=>` compares every base class
subobject, so children of `Printable` couldn't default their
comparisons: `Printable` had no comparison operators of its own, which
made the defaulted operator implicitly deleted. Children that want
member-wise comparison had to write it out by hand instead.
`Printable` is empty, so it now provides comparisons that always compare
equal. Its operands are constrained template parameter rather than
`const Printable&` so that they're only viable for comparing the base
class subobjects themselves. An overload taking `const Printable&` would
also be viable when comparing two `DerivedT` objects by converting them
to the base class, and would then both make children that provide no
comparison silently compare equal and displace the comparisons of
children that provide them through a conversion of their own, as
`EnumBase` does.
Some hand-written comparisons stay, for reasons unrelated to
`Printable`: using `= default` would change their meaning.
Adds `common/ostream_test.cpp`, which covers both the member and friend
forms of defaulting, the resulting comparison categories, and both of
the hazards above.
Assisted-by: Claude Code
---------
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Because we merge cleanups across scopes in various cases, and want to
use linear indexes into the complete stack, the ArrayStack abstraction
is getting in the way more than it's helping. Switch to just a
SmallVector.
This loses the unit testing of the MergeIntoGrandparent logic. This is
covered indirectly by check tests still, but direct testing of it is a
bit tricky given that ScopeStack isn't set up for use without a Context.
Destroy local variables and temporaries at each `}`, and when branching
with `break` and `continue`. In `for` statements, destroy loop variables
along with anything created within the loop at the end of each loop
iteration, and destroy the cursor and range object when the loop
terminates.
Assisted-by: Gemini via Antigravity
---------
Co-authored-by: Geoff Romer <gromer@google.com>
Fix a few API issues. There's also a newly-added file in compiler-rt
that is not supposed to be built by default but is not being excluded
properly by a glob. Added a patch to exclude that and sent
https://github.com/llvm/llvm-project/pull/208861 upstream.
This is based on an idea suggested during review of the original type
erasure PR.
The type-erased check failure path lowers each check through
`CheckFailFormat<Ts...>`, which builds one format adapter per value,
down to the out-of-line `CheckFailImpl`, which takes an array of
base-class `format_adapter*` pointers. Previously a separate variadic
`CheckFailWithAdapters<Adapters...>` template sat in between: it existed
only to bind the adapter temporaries to named parameters so that
pointers to their base class could be collected into a `std::array` and
outlive the call to `CheckFailImpl`.
This removes that layer. `CheckFailFormat` now builds the pointer array
directly, in the braced-init-list of the `CheckFailImpl` call, using a
small `CheckFailFormatAdapterAddr<T>` helper to take the base-class
address of each adapter. The adapter temporaries are materialized as
named parameters of that helper within the same full-expression as the
`CheckFailImpl` call, so they remain alive across the call without a
dedicated function for that purpose.
The win is in debug info, not code. `CheckFailWithAdapters<Adapters...>`
was instantiated once per distinct adapter-type sequence in every
translation unit that uses `CARBON_CHECK`, and each instantiation
carried its own DWARF records -- type entries, string-table offsets, and
range lists. Dropping it removes those records from every such
translation unit. The generated machine code is unchanged.
Measured impact (fastbuild, the inputs to the inner-loop links):
- First-party object files shrink by 173,728 bytes (-0.233%) across the
`carbon-busybox` link inputs; 232 of 270 objects get smaller and none
grow. The largest reductions are in the check-densest translation units
(`type_completion`, `type`, `import_ref`, `function_context`,
`constant`, ...).
- Per `bloaty`, the reduction is almost entirely DWARF (`.debug_info`,
`.debug_str`, `.rela.debug_str_offsets`, `.debug_rnglists`); the loaded
code (VM size) is unchanged.
Assisted-by: Claude Code
---------
Co-authored-by: Geoff Romer <gromer@google.com>
`CARBON_CHECK` and `CARBON_FATAL` messages are formatted with
`llvm::formatv`. Previously each check site that had a message
instantiated its own copy of the formatv machinery -- a `formatv_object`
over a tuple of per-argument format adapters, plus that tuple -- in
every translation unit, keyed on the site's file, line, condition, and
format strings. A translation unit with many checks paid for that
machinery over and over.
This restructures check failure so that the formatting machinery is
compiled exactly once, and only a single small adapter is instantiated
per distinct value type per translation unit:
- `CheckFailImpl` (out-of-line) now takes the message's format string
and an array of already-type-erased `format_adapter`s, and renders the
whole failure message -- prefix plus the extra message -- directly into
one stream. The extra message is rendered in place, so no separate
string is ever materialized for it.
- `FormatvInto` (in the `.cpp`) renders a format string over that
adapter array. Rather than instantiate `llvm::formatv`, it drives the
formatv replacement loop over the public
`formatv_object_base::parseFormatString`, so this rendering code exists
exactly once. (A TODO notes that we should add a type-erased entry point
upstream in LLVM rather than reimplement the loop here.)
- The lowering from the macro down to that out-of-line call is split so
that the only per-check-site instantiation is trivial:
- `CheckFail<...>` is instantiated once per site, since its file, line,
condition, and format template-string parameters are unique to the site.
It just lowers those compile-time strings to ordinary arguments and
forwards to `CheckFailFormat`.
- `CheckFailFormat<Ts...>` is instantiated once per distinct value-type
sequence and shared across sites; it builds one type-erased adapter per
value.
- `CheckFailWithAdapters<Adapters...>` collects pointers to those
adapters into an array and calls `CheckFailImpl`. It is a distinct
function so the adapter temporaries stay alive while pointers to their
base class are in flight.
Format semantics, including runtime format-string validation, are
unchanged, and the rendered message is byte-for-byte identical.
For `DCHECK` in optimized builds the check is dead code; its arguments
are now routed through a trivial `IgnoreDeadCheckArgs` no-op rather than
`CheckFail`. This still type-checks the arguments so they cannot bitrot,
without instantiating any formatting machinery for them and without
provoking unused-variable warnings.
Measured full-rebuild impact (353 first-party translation units,
fastbuild): -112.6s CPU, -6.9% relative to trunk.
Assisted-by: Claude
Because this is an `__attribute__((used))` method in a templated base
class it forces a _huge_ amount of template instantiation in every
translation unit.
Often this was just printing the members of the type, which is still
useful in some cases (such as test output), but adds no value in the
debugger.
A more successful pattern for dumping has been namespace level
functions, and particularly static ones that more transparently don't
expand the non-debugger API surface. Add the few missing functions there
that cover `Printable` types with more interesting contents.
For several of these, it just gives us a "dump the whole thing" function
as a compliment to "dump this entity in the thing". These probably
aren't especially high value, but moving them here they become cheap, so
I've left them in.
For a couple, this expands the rich dumping support of SemIR constructs,
which should be substantially more useful than the previous `Dump`
behavior.
This reduces `check` cumulative object file size by another 14%.
Assisted-by: Antigravity with Gemini
This still only works if the hash of the distinct types are identical
(so it still doesn't address the derived pointer v base pointer case -
well, not in the way we would want to address it, we could use this
change to make derived pointer and base pointer not compare equal, but
that's not very ergonomic)
I think in a follow up maybe I can use a `TranslatingKeyContext` to
translate `Derived*` to `Base*` in general.
No test coverage for this change, since it's a no-compile situation and
we don't seem to generally do no-compile tests.
Discovered while working on #6940
---------
Co-authored-by: Geoff Romer <gromer@google.com>
When the symlink target length from lstat was 0, the code resized the
buffer using status.size() instead of buffer_size, so the first
allocation stayed empty instead of using MinBufferSize. Align the resize
with the buffer_size path used for readlinkat.
This is a mostly routine update, with some edits for a benchmark API
change.
I'm not updating LLVM here, since that could conflict with other ongoing
work.
Most versions are through `pre-commit autoupdate --freeze`, clang-format
was manually updated to the latest at
https://github.com/ssciwr/clang-format-wheel
My read of the style changes here are that they seem fine, none of them
look like regressions (which has caused me to delay/adjust updates in
the past).
The YAML test helpers didn't use the `Printable` abstraction in one
place and instead directly used `<<` with a `std::ostream`. This matches
the `require`s expression in the `error_test_helpers.h` printing logic
for `ErrorOr`, but fails to provide the necessary implementation for
`llvm::formatv` to succeed with the `Yaml::Value` type.
The main fix is to use `Printable` and to define the `Print` method in
terms of `llvm::raw_ostream`. We already have all the mapping hooks in
place to also support `std::ostream` when needed based on that
definition.
This also adds some constraints to the printing in
`error_test_helpers.h` so it is a bit less under-constrained and more
understandable when it is correctly being used. These are just tidying
though, they aren't what makes these headers work together.
I've added a test to try and make sure these test helpers compose as
well.
OwningArrayRef is being removed upstream, per
https://github.com/llvm/llvm-project/pull/169126. This replaces uses
with `SmallVector`.
I've also made a separate commit which does init changes; these aren't
strictly necessary, but I added to make it a little more idiomatic in
spots.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
When building in Bazel actions, notably building runtimes, using
absolute paths makes the results non-hermetic and generally less
cache-friendly.
This restructures the code to only form an absolute path as part of the
`bazel run` change of working directory. It also tries to make the API
for doing this a bit more clear by taking the `exe_path` and
transforming it internally.
To support this, this PR also generalizes the `RemovingDir` to support
relative paths. While these can be tricky -- the working directory needs
to not change while they exist -- that isn't a reason to fully exclude
them and they're useful for implementing relative-path runtimes, etc.
This extracts the C-string `argv`-like building routine to a more
broadly reusable location. It also sinks the verbose logging logic out
of it and into the relevant runners. In turn, it simplifies the verbose
logging logic significantly.
The biggest functional change is removing the implicit synthesis of a
tool's `-v` verbose flag from the presence of a `vlog` stream. I thought
this would be helpful, but in practice of debugging these layers it has
been more of a hindrance than a help -- I pretty often only want verbose
logging on one side or the other, and we have ways of explicitly passing
a `-v` flag to the underlying tools already. I think my instinct to do
this was just wrong, so rip it out and simplify.
This does add an unused feature -- prepending a prefix of arguments
while building the C-string variant. This isn't used in this PR but will
be used in subsequent PRs and it seemed more disruptive to undo that
logic and then re-do it in a later PR. Let me know if it's too confusing
here.
Assisted-by: Gemini Code Assist
---------
Co-authored-by: Geoff Romer <gromer@google.com>
There are two uses I'm not converting here, that seem to want the
"shortest" behavior. For everything else, I'm going to `zip_equal` since
it's more restrictive.
I wish `zip` were named `zip_shortest`.
The standard `std::latch` is very restrictive in how it can be used, and
this makes it hard to easily leverage for simple coordination between a
set of dynamically scheduled tasks, where there isn't an interesting
synchronizing "merge" or future result.
This tool makes it easy to establish a latch, hand out handles to it,
and once all are destroyed, take whatever relevant action.
Note: this is split out of a larger change that uses it. I can wait
until the use case is ready, but seemed nice to review this separately.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
This fixes various violations of C++'s One Definition Rule, where we
accidentally gave the same static data member multiple definitions in
different translation units. Clang happens to emit such definitions with
weak linkage, which allows us to get away with this without link errors,
but it's still formally incorrect.
Also switch keyword order around for a handful of instances of
`constexpr inline`, per agreement in open discussion.
This happens to reduce the size of a `-c dbg` toolchain binary by 7.2
MiB, presumably by making more of our symbols and especially debug info
discardable.
This is just reducing boilerplate in `typed_insts.h` because we have a
number of singleton types, and keep adding more.
The changes to `TemplateString` allow `TemplateString IrName` to be used
as a `StringLiteral`.
---------
Co-authored-by: Dana Jansens <danakj@orodu.net>
Chandler Carruth
Dana Jansens
David BlaikieandGeoff Romer
cui
Jon Ross-Perkins
