Skip to content

Commit e3738eb

Browse files
jonmeowjonmeow
authored
Try out a different IdKind table approach (#5528)
I was thinking about these after #5526, was wondering how others will feel about this kind of approach: - Adding a helper to `TypeEnum` to get the table construction. - In what were previously `Make` functions, return the element instead of returning the table. - By returning the element, no more need to pass in a nullptr (now have a concrete instance). I think this is a mild simplification, but maybe worth it. Note, would appreciate it if there are thoughts on how to provide a boilerplate `Invalid` implementation (maybe it'd be fine to just return `nullptr` and cause a crash that way, but I was hesitant to do that).
1 parent c70f234 commit e3738eb

File tree

4 files changed

+51
-77
lines changed

4 files changed

+51
-77
lines changed

‎toolchain/check/eval.cpp‎

Lines changed: 22 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -669,34 +669,28 @@ static constexpr bool HasGetConstantValueOverload = requires {
669669
using ArgHandlerFnT = auto(EvalContext& context, int32_t arg, Phase* phase)
670670
-> int32_t;
671671

672-
// Returns a lookup table to get constants by Id::Kind. Requires a null IdKind
673-
// as a parameter in order to get the type pack.
672+
// Returns the arg handler for an `IdKind`.
674673
template <typename... Types>
675-
static constexpr auto MakeArgHandlerTable(TypeEnum<Types...>* /*id_kind*/)
676-
-> std::array<ArgHandlerFnT*, SemIR::IdKind::NumValues> {
677-
std::array<ArgHandlerFnT*, SemIR::IdKind::NumValues> table = {};
678-
((table[SemIR::IdKind::template For<Types>.ToIndex()] =
679-
[](EvalContext& eval_context, int32_t arg, Phase* phase) -> int32_t {
680-
auto id = SemIR::Inst::FromRaw<Types>(arg);
681-
if constexpr (HasGetConstantValueOverload<Types>) {
682-
// If we have a custom `GetConstantValue` overload, call it.
683-
return SemIR::Inst::ToRaw(GetConstantValue(eval_context, id, phase));
684-
} else {
685-
// Otherwise, we assume the value is already constant.
686-
return arg;
687-
}
688-
}),
689-
...);
690-
table[SemIR::IdKind::Invalid.ToIndex()] = [](EvalContext& /*context*/,
691-
int32_t /*arg*/,
692-
Phase* /*phase*/) -> int32_t {
693-
CARBON_FATAL("Instruction has argument with invalid IdKind");
694-
};
695-
table[SemIR::IdKind::None.ToIndex()] =
696-
[](EvalContext& /*context*/, int32_t arg, Phase* /*phase*/) -> int32_t {
697-
return arg;
698-
};
699-
return table;
674+
static auto GetArgHandlerFn(TypeEnum<Types...> id_kind) -> ArgHandlerFnT* {
675+
static constexpr std::array<ArgHandlerFnT*, SemIR::IdKind::NumValues> Table =
676+
{
677+
[](EvalContext& eval_context, int32_t arg, Phase* phase) -> int32_t {
678+
auto id = SemIR::Inst::FromRaw<Types>(arg);
679+
if constexpr (HasGetConstantValueOverload<Types>) {
680+
// If we have a custom `GetConstantValue` overload, call it.
681+
return SemIR::Inst::ToRaw(
682+
GetConstantValue(eval_context, id, phase));
683+
} else {
684+
// Otherwise, we assume the value is already constant.
685+
return arg;
686+
}
687+
}...,
688+
// Invalid and None handling (ordering-sensitive).
689+
[](auto...) -> int32_t { CARBON_FATAL("Unexpected invalid IdKind"); },
690+
[](EvalContext& /*context*/, int32_t arg,
691+
Phase* /*phase*/) -> int32_t { return arg; },
692+
};
693+
return Table[id_kind.ToIndex()];
700694
}
701695

702696
// Given the stored value `arg` of an instruction field and its corresponding
@@ -707,9 +701,7 @@ static constexpr auto MakeArgHandlerTable(TypeEnum<Types...>* /*id_kind*/)
707701
static auto GetConstantValueForArg(EvalContext& eval_context,
708702
SemIR::Inst::ArgAndKind arg_and_kind,
709703
Phase* phase) -> int32_t {
710-
static constexpr auto Table =
711-
MakeArgHandlerTable(static_cast<SemIR::IdKind*>(nullptr));
712-
return Table[arg_and_kind.kind().ToIndex()](eval_context,
704+
return GetArgHandlerFn(arg_and_kind.kind())(eval_context,
713705
arg_and_kind.value(), phase);
714706
}
715707

‎toolchain/sem_ir/formatter.cpp‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -906,9 +906,7 @@ auto Formatter::FormatInstLhs(InstId inst_id, Inst inst) -> void {
906906
}
907907

908908
auto Formatter::FormatInstArgAndKind(Inst::ArgAndKind arg_and_kind) -> void {
909-
static constexpr auto Table =
910-
MakeFormatArgFnTable(static_cast<SemIR::IdKind*>(nullptr));
911-
Table[arg_and_kind.kind().ToIndex()](*this, arg_and_kind.value());
909+
GetFormatArgFn(arg_and_kind.kind())(*this, arg_and_kind.value());
912910
}
913911

914912
auto Formatter::FormatInstRhs(Inst inst) -> void {

‎toolchain/sem_ir/formatter.h‎

Lines changed: 17 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -286,15 +286,12 @@ class Formatter {
286286
auto FormatArg(RealId id) -> void;
287287
auto FormatArg(StringLiteralValueId id) -> void;
288288

289-
// For MakeFormatArgFnTable.
289+
// A `FormatArg` wrapper for `FormatInstArgAndKind`.
290290
using FormatArgFnT = auto(Formatter& formatter, int32_t arg) -> void;
291291

292-
// Returns a lookup table to format arguments by their `IdKind`, for
293-
// `FormatInstArgAndKind`. Requires a null IdKind as a parameter in order to
294-
// get the type pack.
292+
// Returns the `FormatArgFnT` for the given `IdKind`.
295293
template <typename... Types>
296-
static constexpr auto MakeFormatArgFnTable(TypeEnum<Types...>* /*id_kind*/)
297-
-> std::array<FormatArgFnT*, SemIR::IdKind::NumValues>;
294+
static auto GetFormatArgFn(TypeEnum<Types...> id_kind) -> FormatArgFnT*;
298295

299296
// Calls `FormatArg` from an `ArgAndKind`.
300297
auto FormatInstArgAndKind(Inst::ArgAndKind arg_and_kind) -> void;
@@ -430,26 +427,21 @@ auto Formatter::FormatEntityStart(llvm::StringRef entity_kind,
430427
}
431428

432429
template <typename... Types>
433-
constexpr auto Formatter::MakeFormatArgFnTable(TypeEnum<Types...>* /*id_kind*/)
434-
-> std::array<FormatArgFnT*, SemIR::IdKind::NumValues> {
435-
std::array<FormatArgFnT*, SemIR::IdKind::NumValues> table = {};
436-
((table[SemIR::IdKind::template For<Types>.ToIndex()] =
437-
[](Formatter& formatter, int32_t arg) -> void {
438-
auto typed_arg = SemIR::Inst::FromRaw<Types>(arg);
439-
if constexpr (requires { formatter.FormatArg(typed_arg); }) {
440-
formatter.FormatArg(typed_arg);
441-
} else {
442-
CARBON_FATAL("Missing FormatArg for {0}", typeid(Types).name());
443-
}
444-
}),
445-
...);
446-
table[SemIR::IdKind::Invalid.ToIndex()] = [](Formatter& /*formatter*/,
447-
int32_t /*arg*/) -> void {
448-
CARBON_FATAL("Instruction has argument with invalid IdKind");
430+
auto Formatter::GetFormatArgFn(TypeEnum<Types...> id_kind) -> FormatArgFnT* {
431+
static constexpr std::array<FormatArgFnT*, IdKind::NumValues> Table = {
432+
[](Formatter& formatter, int32_t arg) -> void {
433+
auto typed_arg = Inst::FromRaw<Types>(arg);
434+
if constexpr (requires { formatter.FormatArg(typed_arg); }) {
435+
formatter.FormatArg(typed_arg);
436+
} else {
437+
CARBON_FATAL("Missing FormatArg for {0}", typeid(Types).name());
438+
}
439+
}...,
440+
// Invalid and None handling (ordering-sensitive).
441+
[](auto...) -> void { CARBON_FATAL("Unexpected invalid IdKind"); },
442+
[](auto...) -> void {},
449443
};
450-
table[SemIR::IdKind::None.ToIndex()] = [](Formatter& /*formatter*/,
451-
int32_t /*arg*/) -> void {};
452-
return table;
444+
return Table[id_kind.ToIndex()];
453445
}
454446

455447
} // namespace Carbon::SemIR

‎toolchain/sem_ir/inst_fingerprinter.cpp‎

Lines changed: 11 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -314,31 +314,23 @@ struct Worklist {
314314

315315
using AddFnT = auto(Worklist& worklist, int32_t arg) -> void;
316316

317-
// Returns a lookup table to add an argument of the given kind. Requires a
318-
// null IdKind as a parameter in order to get the type pack.
317+
// Returns the arg handler for an `IdKind`.
319318
template <typename... Types>
320-
static constexpr auto MakeAddTable(TypeEnum<Types...>* /*id_kind*/)
321-
-> std::array<AddFnT*, IdKind::NumValues> {
322-
std::array<AddFnT*, IdKind::NumValues> table = {};
323-
((table[IdKind::template For<Types>.ToIndex()] =
324-
[](Worklist& worklist, int32_t arg) {
325-
worklist.Add(Inst::FromRaw<Types>(arg));
326-
}),
327-
...);
328-
table[IdKind::Invalid.ToIndex()] = [](Worklist& /*worklist*/,
329-
int32_t /*arg*/) {
330-
CARBON_FATAL("Unexpected invalid argument kind");
319+
static auto GetAddFn(TypeEnum<Types...> id_kind) -> AddFnT* {
320+
static constexpr std::array<AddFnT*, IdKind::NumValues> Table = {
321+
[](Worklist& worklist, int32_t arg) {
322+
worklist.Add(Inst::FromRaw<Types>(arg));
323+
}...,
324+
// Invalid and None handling (ordering-sensitive).
325+
[](auto...) { CARBON_FATAL("Unexpected invalid IdKind"); },
326+
[](auto...) {},
331327
};
332-
table[IdKind::None.ToIndex()] = [](Worklist& /*worklist*/,
333-
int32_t /*arg*/) {};
334-
return table;
328+
return Table[id_kind.ToIndex()];
335329
}
336330

337331
// Add an instruction argument to the contents of the current instruction.
338332
auto AddWithKind(Inst::ArgAndKind arg) -> void {
339-
static constexpr auto Table = MakeAddTable(static_cast<IdKind*>(nullptr));
340-
341-
Table[arg.kind().ToIndex()](*this, arg.value());
333+
GetAddFn(arg.kind())(*this, arg.value());
342334
}
343335

344336
// Ensure all the instructions on the todo list have fingerprints. To avoid a

0 commit comments

Comments
 (0)