Skip to content

Dump CheckIRId more explicitly for InstId tags #6172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 6 commits into from
+350 −281
Closed

Dump CheckIRId more explicitly for InstId tags #6172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
567d7a9
debug-irs
danakj Oct 6, 2025
5b5b7b6
review
danakj Oct 7, 2025
d3d29f9
wording
danakj Oct 7, 2025
df40a92
rename-irs
danakj Oct 7, 2025
b3726b9
review
danakj Oct 7, 2025
82f33c8
slightly-more-typesafe
danakj Oct 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 27 additions & 16 deletions scripts/lldbinit.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,16 @@ def print_usage() -> None:
Dumps the value of an associated ID, using the C++ Dump() functions.

Usage:
dump <CONTEXT> [<EXPR>|-- <EXPR>|<TYPE><ID>|<TYPE> <ID>]
dump <CONTEXT> [<EXPR>|-- <EXPR>|[<IR>.]<TYPE><ID>]

Args:
CONTEXT is the dump context, such a SemIR::Context reference, a SemIR::File,
a Parse::Context, or a Lex::TokenizeBuffer.
EXPR is a C++ expression such as a variable name. Use `--` to prevent it from
being treated as a TYPE and ID.
IR is the CheckIRId(N) in the form `irN`. Not all ID types have an `ir`
prefix, the ones which do are dumped with that prefix, such as for
`inst` as `ir1.inst2`.
TYPE can be `inst`, `constant`, `generic`, `impl`, `entity_name`, etc. See
the `Label` string in `IdBase` classes to find possible TYPE names,
though only Id types that have a matching `Make...Id()` function are
Expand All @@ -74,8 +77,13 @@ def print_usage() -> None:

context = args[0]

# The set of "Make" functions in dump.cpp.
# The set of "Make" functions in dump.cpp. These use ADL to find the factory
# function in `check/` or in `sem_ir/`.
id_types = {
"inst": "MakeInstId",
}
# These id types don't have an IdTag embedded in them.
untagged_id_types = {
"class": "SemIR::MakeClassId",
"constant": "SemIR::MakeConstantId",
"symbolic_constant": "SemIR::MakeSymbolicConstantId",
Expand All @@ -85,7 +93,6 @@ def print_usage() -> None:
"generic": "SemIR::MakeGenericId",
"impl": "SemIR::MakeImplId",
"inst_block": "SemIR::MakeInstBlockId",
"inst": "SemIR::MakeInstId",
"interface": "SemIR::MakeInterfaceId",
"name": "SemIR::MakeNameId",
"name_scope": "SemIR::MakeNameScopeId",
Expand All @@ -110,27 +117,31 @@ def print_dump(context: str, expr: str) -> None:

# Try to find a type + id from the input args. If not, the id will be passed
# through directly to C++, as it can be a variable name.
id_type = None
found_id_type = False

# Look for <type><id> as a single argument.
if m := re.fullmatch("([a-z_]+)(\\d+)", args[1]):
if m[1] in id_types:
# Look for <irN>.<type><id> as a single argument.
if m := re.fullmatch("ir(\\d+)\\.([a-z_]+)(\\d+)", args[1]):
if m[2] in id_types:
if len(args) != 2:
print_usage()
return
id_type = m[1]
print_dump(context, f"{id_types[id_type]}({m[2]})")
id_type = m[2]
print_dump(
context, f"{id_types[id_type]}({context}, {m[1]}, {m[3]})"
)
found_id_type = True

# Look for <type> <id> as two arguments.
if not id_type:
if args[1] in id_types:
if len(args) != 3:
# Look for <type><id> as a single argument.
if m := re.fullmatch("([a-z_]+)(\\d+)", args[1]):
if m[1] in untagged_id_types:
if len(args) != 2:
print_usage()
return
id_type = args[1]
print_dump(context, f"{id_types[id_type]}({args[2]})")
id_type = m[1]
print_dump(context, f"{untagged_id_types[id_type]}({m[2]})")
found_id_type = True

if not id_type:
if not found_id_type:
# Use `--` to escape a variable name like `inst22`.
if args[1] == "--":
expr = " ".join(args[2:])
Expand Down
113 changes: 71 additions & 42 deletions toolchain/base/value_store.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,37 +33,71 @@ class ValueStoreNotPrintable {};
} // namespace Internal

struct IdTag {
IdTag()
: id_tag_(0),
initial_reserved_ids_(std::numeric_limits<int32_t>::max()) {}
IdTag() = default;

explicit IdTag(int32_t id_index, int32_t initial_reserved_ids)
: initial_reserved_ids_(initial_reserved_ids) {
// Shift down by 1 to get out of the high bit to avoid using any negative
// ids, since they have special uses.
// Shift down by another 1 to free up the second highest bit for a marker to
// indicate whether the index is tagged (& needs to be untagged) or not.
// Add one to the index so it's not zero-based, to make it a bit less likely
// this doesn't collide with anything else (though with the
// second-highest-bit-tagging this might not be needed).
id_tag_ = llvm::reverseBits((((id_index + 1) << 1) | 1) << 1);
}
auto GetCheckIRId() const -> int32_t {
return (llvm::reverseBits(id_tag_) >> 2) - 1;
}
: // Shift down by 1 to get out of the high bit to avoid using any
// negative ids, since they have special uses. Shift down by another 1
// to free up the second highest bit for a marker to indicate whether
// the index is tagged (& needs to be untagged) or not. Add one to the
// index so it's not zero-based, to make it a bit less likely this
// doesn't collide with anything else (though with the
// second-highest-bit-tagging this might not be needed).
id_tag_(llvm::reverseBits((((id_index + 1) << 1) | 1) << 1)),
initial_reserved_ids_(initial_reserved_ids) {}

auto Apply(int32_t index) const -> int32_t {
if (index < initial_reserved_ids_) {
return index;
}
// assert that id_tag_ doesn't have the second highest bit set
// TODO: Assert that id_tag_ doesn't have the second highest bit set.
return index ^ id_tag_;
}

auto Remove(int32_t tagged_index) const -> int32_t {
if (!HasTag(tagged_index)) {
CARBON_DCHECK(tagged_index < initial_reserved_ids_,
"This untagged index is outside the initial reserved ids "
"and should have been tagged.");
return tagged_index;
}
auto index = tagged_index ^ id_tag_;
CARBON_DCHECK(index >= initial_reserved_ids_,
"When removing tagging bits, found an index that "
"shouldn't've been tagged in the first place.");
return index;
}

// Gets the value unique to this IdTag instance that is added to indices in
// Apply, and removed in Remove.
auto GetContainerTag() const -> int32_t {
return (llvm::reverseBits(id_tag_) >> 2) - 1;
}

// Returns whether `tagged_index` has an IdTag applied to it, from this IdTag
// instance or any other one.
static auto HasTag(int32_t tagged_index) -> bool {
return (llvm::reverseBits(2) & tagged_index) != 0;
}

template <class TagT>
struct TagAndIndex {
TagT tag;
int32_t index;
};

template <typename TagT>
static auto DecomposeWithBestEffort(int32_t tagged_index)
-> std::pair<int32_t, int32_t> {
-> TagAndIndex<TagT> {
if (tagged_index < 0) {
return {-1, tagged_index};
// TODO: This should return TagT::None, but we need a fallback TagT other
// than `int32_t`.
return {TagT{-1}, tagged_index};
}
if ((llvm::reverseBits(2) & tagged_index) == 0) {
return {-1, tagged_index};
if (!HasTag(tagged_index)) {
// TODO: This should return TagT::None, but we need a fallback TagT other
// than `int32_t`.
return {TagT{-1}, tagged_index};
}
int length = 0;
int location = 0;
Expand All @@ -83,28 +117,20 @@ struct IdTag {
}
}
if (length < 8) {
return {-1, tagged_index};
// TODO: This should return TagT::None, but we need a fallback TagT other
// than `int32_t`.
return {TagT{-1}, tagged_index};
}
auto index_mask = llvm::maskTrailingOnes<uint32_t>(location);
auto ir_id = (llvm::reverseBits(tagged_index & ~index_mask) >> 2) - 1;
auto tag = (llvm::reverseBits(tagged_index & ~index_mask) >> 2) - 1;
auto index = tagged_index & index_mask;
return {ir_id, index};
}
auto Remove(int32_t tagged_index) const -> int32_t {
if ((llvm::reverseBits(2) & tagged_index) == 0) {
CARBON_DCHECK(tagged_index < initial_reserved_ids_,
"This untagged index is outside the initial reserved ids "
"and should have been tagged.");
return tagged_index;
}
auto index = tagged_index ^ id_tag_;
CARBON_DCHECK(index >= initial_reserved_ids_,
"When removing tagging bits, found an index that "
"shouldn't've been tagged in the first place.");
return index;
return {.tag = TagT{static_cast<int32_t>(tag)},
.index = static_cast<int32_t>(index)};
}
int32_t id_tag_;
int32_t initial_reserved_ids_;

private:
int32_t id_tag_ = 0;
int32_t initial_reserved_ids_ = std::numeric_limits<int32_t>::max();
Comment on lines +132 to +133
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you switch away from the prior setup? It seemed safer, since there are only two constructors and both must set different values.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think setting things on the fields is generally safer and preferred. It's easy to miss a field in the other form. If you do, this way they all get initialized to some default value at least.

};

// A simple wrapper for accumulating values, providing IDs to later retrieve the
Expand Down Expand Up @@ -279,13 +305,16 @@ class ValueStore
// Attempt to decompose id.index to include extra detail in the check here
#ifndef NDEBUG
if (index >= size_) {
auto [ir_id, decomposed_index] = IdTag::DecomposeWithBestEffort(id.index);
// TODO: Could we add InstId::TagType = CheckIRId and use that here to
// print the ChcekIRId properly?
auto [ir_id, decomposed_index] =
IdTag::DecomposeWithBestEffort<int32_t>(id.index);
CARBON_DCHECK(
index < size_,
"Untagged index was outside of container range. Possibly tagged "
"index {0}. Best-effort decomposition: CheckIRId: {1}, index: {2}. "
"index {0}. Best-effort decomposition: Tag: {1}, index: {2}. "
"The IdTag for this container is: {3}",
id.index, ir_id, decomposed_index, tag_.GetCheckIRId());
id.index, ir_id, decomposed_index, tag_.GetContainerTag());
}
#endif
return index;
Expand Down
1 change: 1 addition & 0 deletions toolchain/check/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ cc_library(
"//toolchain/parse:tree",
"//toolchain/sem_ir:dump",
"//toolchain/sem_ir:file",
"//toolchain/sem_ir:typed_insts",
],
# Always link dump methods.
alwayslink = 1,
Expand Down
1 change: 1 addition & 0 deletions toolchain/check/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ class Context {
// Directly expose SemIR::File data accessors for brevity in calls.
// --------------------------------------------------------------------------

auto check_ir_id() -> SemIR::CheckIRId { return sem_ir().check_ir_id(); }
auto identifiers() -> SharedValueStores::IdentifierStore& {
return sem_ir().identifiers();
}
Expand Down
12 changes: 12 additions & 0 deletions toolchain/check/dump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "toolchain/parse/tree.h"
#include "toolchain/sem_ir/dump.h"
#include "toolchain/sem_ir/file.h"
#include "toolchain/sem_ir/ids.h"

namespace Carbon::Check {

Expand Down Expand Up @@ -138,6 +139,17 @@ LLVM_DUMP_METHOD static auto Dump(const Context& context, SemIR::TypeId type_id)
return SemIR::Dump(context.sem_ir(), type_id);
}

LLVM_DUMP_METHOD static auto MakeInstId(Context& context, int check_ir_id,
int id) -> SemIR::InstId {
if (SemIR::CheckIRId(check_ir_id) == context.check_ir_id()) {
return SemIR::MakeInstId(context.sem_ir(), check_ir_id, id);
} else {
const auto& import_ir = context.import_irs().Get(
context.check_ir_map().Get(SemIR::CheckIRId(check_ir_id)));
return SemIR::MakeInstId(*import_ir.sem_ir, check_ir_id, id);
}
}

} // namespace Carbon::Check

#endif // NDEBUG
4 changes: 2 additions & 2 deletions toolchain/check/testdata/basics/raw_sem_ir/builtins.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
// CHECK:STDOUT: filename: builtins.carbon
// CHECK:STDOUT: sem_ir:
// CHECK:STDOUT: import_irs:
// CHECK:STDOUT: ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir_insts: {}
// CHECK:STDOUT: clang_decls: {}
// CHECK:STDOUT: name_scopes:
Expand Down
6 changes: 3 additions & 3 deletions toolchain/check/testdata/basics/raw_sem_ir/cpp_interop.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ fn G(x: Cpp.X) {
// CHECK:STDOUT: filename: import.carbon
// CHECK:STDOUT: sem_ir:
// CHECK:STDOUT: import_irs:
// CHECK:STDOUT: ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir_insts:
// CHECK:STDOUT: import_ir_inst0: {ir_id: ir1, clang_source_loc_id: clang_source_loc0}
// CHECK:STDOUT: import_ir_inst0: {ir_id: import_ir1, clang_source_loc_id: clang_source_loc0}
// CHECK:STDOUT: clang_decls:
// CHECK:STDOUT: clang_decl_id0: {key: "<translation unit>", inst_id: ir0.inst16}
// CHECK:STDOUT: clang_decl_id1: {key: "struct X {}", inst_id: ir0.inst18}
Expand Down
14 changes: 7 additions & 7 deletions toolchain/check/testdata/basics/raw_sem_ir/multifile.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ fn B() {
// CHECK:STDOUT: filename: a.carbon
// CHECK:STDOUT: sem_ir:
// CHECK:STDOUT: import_irs:
// CHECK:STDOUT: ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir_insts: {}
// CHECK:STDOUT: clang_decls: {}
// CHECK:STDOUT: name_scopes:
Expand Down Expand Up @@ -88,12 +88,12 @@ fn B() {
// CHECK:STDOUT: filename: b.carbon
// CHECK:STDOUT: sem_ir:
// CHECK:STDOUT: import_irs:
// CHECK:STDOUT: ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: ir2: {decl_id: ir1.inst15, is_export: false}
// CHECK:STDOUT: import_ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir2: {decl_id: ir1.inst15, is_export: false}
// CHECK:STDOUT: import_ir_insts:
// CHECK:STDOUT: import_ir_inst0: {ir_id: ir2, inst_id: ir0.inst15}
// CHECK:STDOUT: import_ir_inst1: {ir_id: ir2, inst_id: ir0.inst15}
// CHECK:STDOUT: import_ir_inst0: {ir_id: import_ir2, inst_id: ir0.inst15}
// CHECK:STDOUT: import_ir_inst1: {ir_id: import_ir2, inst_id: ir0.inst15}
// CHECK:STDOUT: clang_decls: {}
// CHECK:STDOUT: name_scopes:
// CHECK:STDOUT: name_scope0: {inst: inst14, parent_scope: name_scope<none>, has_error: false, extended_scopes: [], names: {name1: ir1.inst16, name0: ir1.inst17}}
Expand Down
14 changes: 7 additions & 7 deletions toolchain/check/testdata/basics/raw_sem_ir/multifile_with_textual_ir.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ fn B() {
// CHECK:STDOUT: filename: a.carbon
// CHECK:STDOUT: sem_ir:
// CHECK:STDOUT: import_irs:
// CHECK:STDOUT: ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir_insts: {}
// CHECK:STDOUT: clang_decls: {}
// CHECK:STDOUT: name_scopes:
Expand Down Expand Up @@ -107,12 +107,12 @@ fn B() {
// CHECK:STDOUT: filename: b.carbon
// CHECK:STDOUT: sem_ir:
// CHECK:STDOUT: import_irs:
// CHECK:STDOUT: ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: ir2: {decl_id: ir1.inst15, is_export: false}
// CHECK:STDOUT: import_ir0: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir1: {decl_id: inst<none>, is_export: false}
// CHECK:STDOUT: import_ir2: {decl_id: ir1.inst15, is_export: false}
// CHECK:STDOUT: import_ir_insts:
// CHECK:STDOUT: import_ir_inst0: {ir_id: ir2, inst_id: ir0.inst15}
// CHECK:STDOUT: import_ir_inst1: {ir_id: ir2, inst_id: ir0.inst15}
// CHECK:STDOUT: import_ir_inst0: {ir_id: import_ir2, inst_id: ir0.inst15}
// CHECK:STDOUT: import_ir_inst1: {ir_id: import_ir2, inst_id: ir0.inst15}
// CHECK:STDOUT: clang_decls: {}
// CHECK:STDOUT: name_scopes:
// CHECK:STDOUT: name_scope0: {inst: inst14, parent_scope: name_scope<none>, has_error: false, extended_scopes: [], names: {name1: ir1.inst16, name0: ir1.inst17}}
Expand Down
Loading
Loading