Skip to content

Commit

Permalink
Fix DSLX interpreter misbehavior with multiply instantiated procs.
Browse files Browse the repository at this point in the history 
Previously, the channel object was associated with the type information. This posed two potential problems: multiply instantiated procs may not be handled properly because there could be one channel object shared by multiple instantiations, and, channels may not be cleared between intepreter invocations. This change address that issue by changing InterpValue to hold immutable channel references instead of the channel object itself. The channel references include an optional channel instance id uniquely identifying the channel in the hieararchy.

Also, use this opportunity to clean up the proc interpreter code quite a bit. Previously, the interface was quite messy and unclear. Now a single object `ProcHiearchyInterpreter` contains all necessary objects and provide simple Tick and TickUntilOutput methods similar to the IR interpreter.

Further collateral cleanup damage is making the trace_channels output and deadlock output clearer with respect to where the channels are in the hierarchy.

Fixes #1789

PiperOrigin-RevId: 709088443
  • Loading branch information
@meheffernan @copybara-github
meheffernan authored and copybara-github committed Dec 23, 2024
1 parent 1554191 commit 5f9cd3b
Show file tree
Hide file tree
Showing 28 changed files with 1,971 additions and 1,289 deletions.
5 changes: 4 additions & 1 deletion xls/dslx/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,9 @@ cc_library(
":errors",
":import_data",
":interp_value",
":interp_value_utils",
":warning_collector",
":warning_kind",
"//xls/common:casts",
"//xls/common:visitor",
"//xls/common/status:ret_check",
"//xls/common/status:status_macros",
Expand Down Expand Up @@ -169,6 +169,7 @@ cc_library(
srcs = ["interp_value_utils.cc"],
hdrs = ["interp_value_utils.h"],
deps = [
":channel_direction",
":interp_value",
"//xls/common/status:ret_check",
"//xls/common/status:status_macros",
Expand All @@ -178,6 +179,7 @@ cc_library(
"//xls/ir:bits_ops",
"//xls/ir:ir_parser",
"//xls/ir:value",
"@com_google_absl//absl/functional:function_ref",
"@com_google_absl//absl/log:check",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
Expand Down Expand Up @@ -237,6 +239,7 @@ cc_library(
srcs = ["interp_value.cc"],
hdrs = ["interp_value.h"],
deps = [
":channel_direction",
":dslx_builtins",
":value_format_descriptor",
"//xls/common:math_util",
Expand Down
39 changes: 35 additions & 4 deletions xls/dslx/bytecode/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ cc_library(
"//xls/ir:format_strings",
"@com_google_absl//absl/cleanup",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/functional:function_ref",
"@com_google_absl//absl/log",
"@com_google_absl//absl/log:check",
"@com_google_absl//absl/status",
Expand Down Expand Up @@ -257,7 +258,6 @@ cc_library(
":builtins",
":bytecode",
":bytecode_cache_interface",
":bytecode_emitter",
":bytecode_interpreter_options",
":frame",
":interpreter_stack",
Expand All @@ -272,7 +272,6 @@ cc_library(
"//xls/dslx:value_format_descriptor",
"//xls/dslx/frontend:ast",
"//xls/dslx/frontend:pos",
"//xls/dslx/frontend:proc",
"//xls/dslx/frontend:proc_id",
"//xls/dslx/type_system:parametric_env",
"//xls/dslx/type_system:type",
Expand All @@ -295,6 +294,40 @@ cc_library(
],
)

cc_library(
name = "proc_hierarchy_interpreter",
srcs = ["proc_hierarchy_interpreter.cc"],
hdrs = ["proc_hierarchy_interpreter.h"],
deps = [
":bytecode",
":bytecode_emitter",
":bytecode_interpreter",
":bytecode_interpreter_options",
":frame",
":interpreter_stack",
"//xls/common/status:ret_check",
"//xls/common/status:status_macros",
"//xls/dslx:channel_direction",
"//xls/dslx:import_data",
"//xls/dslx:interp_value",
"//xls/dslx:interp_value_utils",
"//xls/dslx/frontend:ast",
"//xls/dslx/frontend:pos",
"//xls/dslx/frontend:proc",
"//xls/dslx/frontend:proc_id",
"//xls/dslx/type_system:parametric_env",
"//xls/dslx/type_system:type",
"//xls/dslx/type_system:type_info",
"@com_google_absl//absl/log",
"@com_google_absl//absl/log:check",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
"@com_google_absl//absl/strings",
"@com_google_absl//absl/strings:str_format",
"@com_google_absl//absl/types:span",
],
)

cc_test(
name = "bytecode_interpreter_test",
srcs = ["bytecode_interpreter_test.cc"],
Expand All @@ -306,7 +339,6 @@ cc_test(
":bytecode_interpreter_options",
":interpreter_stack",
"//xls/common:xls_gunit_main",
"//xls/common/file:temp_file",
"//xls/common/status:matchers",
"//xls/common/status:ret_check",
"//xls/common/status:status_macros",
Expand All @@ -318,7 +350,6 @@ cc_test(
"//xls/dslx:virtualizable_file_system",
"//xls/dslx/frontend:ast",
"//xls/dslx/frontend:pos",
"//xls/dslx/frontend:proc_id",
"//xls/dslx/run_routines",
"//xls/dslx/type_system:parametric_env",
"//xls/dslx/type_system:type_info",
Expand Down
68 changes: 46 additions & 22 deletions xls/dslx/bytecode/bytecode_emitter.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include "absl/cleanup/cleanup.h"
#include "absl/container/flat_hash_map.h"
#include "absl/functional/function_ref.h"
#include "absl/log/check.h"
#include "absl/log/log.h"
#include "absl/status/status.h"
Expand Down Expand Up @@ -66,16 +67,6 @@
namespace xls::dslx {
namespace {

// Determines the owning proc for `node` by walking the parent links.
const Proc* GetContainingProc(const AstNode* node) {
AstNode* proc_node = node->parent();
while (dynamic_cast<const Proc*>(proc_node) == nullptr) {
proc_node = proc_node->parent();
CHECK(proc_node != nullptr);
}
return dynamic_cast<const Proc*>(proc_node);
}

// Find concrete type of channel's payload.
absl::StatusOr<std::unique_ptr<Type>> GetChannelPayloadType(
const TypeInfo* type_info, const Expr* channel) {
Expand Down Expand Up @@ -106,12 +97,9 @@ absl::StatusOr<Bytecode::ChannelData> CreateChannelData(
MakeValueFormatDescriptor(*channel_payload_type.get(),
format_preference));

const Proc* proc_node = GetContainingProc(channel);
std::string_view proc_name = proc_node->identifier();

return Bytecode::ChannelData(
absl::StrFormat("%s::%s", proc_name, channel->ToString()),
std::move(channel_payload_type), std::move(struct_fmt_desc));
return Bytecode::ChannelData(channel->ToString(),
std::move(channel_payload_type),
std::move(struct_fmt_desc));
}

absl::StatusOr<ValueFormatDescriptor> ExprToValueFormatDescriptor(
Expand Down Expand Up @@ -148,10 +136,12 @@ std::optional<ValueFormatDescriptor> GetFormatDescriptorFromNumber(
BytecodeEmitter::BytecodeEmitter(
ImportData* import_data, const TypeInfo* type_info,
const std::optional<ParametricEnv>& caller_bindings,
std::optional<absl::FunctionRef<int64_t()>> channel_instance_allocator,
const BytecodeEmitterOptions& options)
: import_data_(import_data),
type_info_(type_info),
caller_bindings_(caller_bindings),
channel_instance_allocator_(channel_instance_allocator),
options_(options) {}

BytecodeEmitter::~BytecodeEmitter() = default;
Expand All @@ -169,8 +159,19 @@ BytecodeEmitter::Emit(ImportData* import_data, const TypeInfo* type_info,
const Function& f,
const std::optional<ParametricEnv>& caller_bindings,
const BytecodeEmitterOptions& options) {
return EmitProcNext(import_data, type_info, f, caller_bindings,
/*proc_members=*/{}, options);
return EmitInternal(import_data, type_info, f, caller_bindings,
/*proc_members=*/{},
/*channel_instance_allocator=*/std::nullopt, options);
}

/* static */ absl::StatusOr<std::unique_ptr<BytecodeFunction>>
BytecodeEmitter::EmitProcConfig(
ImportData* import_data, const TypeInfo* type_info, const Function& f,
const std::optional<ParametricEnv>& caller_bindings,
std::optional<absl::FunctionRef<int64_t()>> channel_instance_allocator,
const BytecodeEmitterOptions& options) {
return EmitInternal(import_data, type_info, f, caller_bindings,
/*proc_members=*/{}, channel_instance_allocator, options);
}

/* static */ absl::StatusOr<std::unique_ptr<BytecodeFunction>>
Expand All @@ -179,9 +180,21 @@ BytecodeEmitter::EmitProcNext(
const std::optional<ParametricEnv>& caller_bindings,
const std::vector<NameDef*>& proc_members,
const BytecodeEmitterOptions& options) {
return EmitInternal(import_data, type_info, f, caller_bindings, proc_members,
/*channel_instance_allocator=*/std::nullopt, options);
}

/* static */ absl::StatusOr<std::unique_ptr<BytecodeFunction>>
BytecodeEmitter::EmitInternal(
ImportData* import_data, const TypeInfo* type_info, const Function& f,
const std::optional<ParametricEnv>& caller_bindings,
const std::vector<NameDef*>& proc_members,
std::optional<absl::FunctionRef<int64_t()>> channel_instance_allocator,
const BytecodeEmitterOptions& options) {
XLS_RET_CHECK(type_info != nullptr);

BytecodeEmitter emitter(import_data, type_info, caller_bindings, options);
BytecodeEmitter emitter(import_data, type_info, caller_bindings,
channel_instance_allocator, options);
for (const NameDef* name_def : proc_members) {
emitter.namedef_to_slot_[name_def] = emitter.next_slotno_++;
}
Expand All @@ -198,7 +211,8 @@ BytecodeEmitter::EmitExpression(
const absl::flat_hash_map<std::string, InterpValue>& env,
const std::optional<ParametricEnv>& caller_bindings,
const BytecodeEmitterOptions& options) {
BytecodeEmitter emitter(import_data, type_info, caller_bindings, options);
BytecodeEmitter emitter(import_data, type_info, caller_bindings,
/*channel_instance_allocator=*/std::nullopt, options);

XLS_ASSIGN_OR_RETURN(std::vector<const NameDef*> name_defs,
CollectReferencedUnder(expr));
Expand Down Expand Up @@ -703,8 +717,18 @@ absl::Status BytecodeEmitter::HandleChannelDecl(const ChannelDecl* node) {
// Channels are created as constexpr values during type deduction/constexpr
// evaluation, since they're concrete values that need to be shared amongst
// two actors.
XLS_ASSIGN_OR_RETURN(InterpValue channel, type_info_->GetConstExpr(node));
Add(Bytecode::MakeLiteral(node->span(), channel));
std::optional<Type*> maybe_decl_type = type_info_->GetItem(node);
auto* tuple_type = dynamic_cast<TupleType*>(maybe_decl_type.value());
XLS_RET_CHECK(tuple_type != nullptr);
XLS_RET_CHECK_EQ(tuple_type->size(), 2);

XLS_ASSIGN_OR_RETURN(auto in_out_channels,
CreateChannelReferencePair(&tuple_type->GetMemberType(0),
channel_instance_allocator_));

Add(Bytecode::MakeLiteral(
node->span(),
InterpValue::MakeTuple({in_out_channels.first, in_out_channels.second})));
return absl::OkStatus();
}

Expand Down
24 changes: 21 additions & 3 deletions xls/dslx/bytecode/bytecode_emitter.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <vector>

#include "absl/container/flat_hash_map.h"
#include "absl/functional/function_ref.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
#include "xls/dslx/bytecode/bytecode.h"
Expand Down Expand Up @@ -61,6 +62,13 @@ class BytecodeEmitter : public ExprVisitor {
const std::optional<ParametricEnv>& caller_bindings,
const BytecodeEmitterOptions& options = BytecodeEmitterOptions());

static absl::StatusOr<std::unique_ptr<BytecodeFunction>> EmitProcConfig(
ImportData* import_data, const TypeInfo* type_info, const Function& f,
const std::optional<ParametricEnv>& caller_bindings,
std::optional<absl::FunctionRef<int64_t()>> channel_instance_allocator =
std::nullopt,
const BytecodeEmitterOptions& options = BytecodeEmitterOptions());

// Emits a function, just as the above, but reserves the first N slots for
// the given proc members.
static absl::StatusOr<std::unique_ptr<BytecodeFunction>> EmitProcNext(
Expand All @@ -70,11 +78,20 @@ class BytecodeEmitter : public ExprVisitor {
const BytecodeEmitterOptions& options = BytecodeEmitterOptions());

private:
BytecodeEmitter(ImportData* import_data, const TypeInfo* type_info,
const std::optional<ParametricEnv>& caller_bindings,
const BytecodeEmitterOptions& options);
BytecodeEmitter(
ImportData* import_data, const TypeInfo* type_info,
const std::optional<ParametricEnv>& caller_bindings,
std::optional<absl::FunctionRef<int64_t()>> channel_instance_allocator,
const BytecodeEmitterOptions& options);
~BytecodeEmitter() override;

static absl::StatusOr<std::unique_ptr<BytecodeFunction>> EmitInternal(
ImportData* import_data, const TypeInfo* type_info, const Function& f,
const std::optional<ParametricEnv>& caller_bindings,
const std::vector<NameDef*>& proc_members,
std::optional<absl::FunctionRef<int64_t()>> channel_instance_allocator,
const BytecodeEmitterOptions& options = BytecodeEmitterOptions());

// Initializes namedef-to-slot mapping.
absl::Status Init(const Function& f);

Expand Down Expand Up @@ -153,6 +170,7 @@ class BytecodeEmitter : public ExprVisitor {
ImportData* import_data_;
const TypeInfo* type_info_;
const std::optional<ParametricEnv>& caller_bindings_;
std::optional<absl::FunctionRef<int64_t()>> channel_instance_allocator_;
BytecodeEmitterOptions options_;

std::vector<Bytecode> bytecode_;
Expand Down
12 changes: 7 additions & 5 deletions xls/dslx/bytecode/bytecode_emitter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1567,7 +1567,8 @@ proc Foo {
const std::vector<Bytecode>& config_bytecodes = bf->bytecodes();
ASSERT_EQ(config_bytecodes.size(), 7);
const std::vector<std::string> kConfigExpected = {
"literal (channel, channel)",
"literal (channel_reference(out, channel_instance_id=none), "
"channel_reference(in, channel_instance_id=none))",
"expand_tuple",
"store 0",
"store 1",
Expand Down Expand Up @@ -1634,12 +1635,13 @@ proc Parent {
tm.type_info->GetTopLevelProcTypeInfo(parent));
XLS_ASSERT_OK_AND_ASSIGN(
std::unique_ptr<BytecodeFunction> parent_config_bf,
BytecodeEmitter::Emit(&import_data, parent_ti, parent->config(),
ParametricEnv()));
BytecodeEmitter::EmitProcConfig(&import_data, parent_ti, parent->config(),
ParametricEnv()));
const std::vector<Bytecode>& parent_config_bytecodes =
parent_config_bf->bytecodes();
const std::vector<std::string> kParentConfigExpected = {
"literal (channel, channel)",
"literal (channel_reference(out, channel_instance_id=none), "
"channel_reference(in, channel_instance_id=none))",
"expand_tuple",
"store 0",
"store 1",
Expand Down Expand Up @@ -1701,7 +1703,7 @@ proc Parent {
"load 0", //
"literal u1:1", //
"literal u32:0", //
"recv Child::c", //
"recv c", //
"expand_tuple", //
"store 4", //
"store 5", //
Expand Down
Loading

0 comments on commit 5f9cd3b

Please sign in to comment.