Skip to content

fix: DSLX C++ transpiler must unique-ify generated temp variable names to not collide with DSLX struct member field names #3741

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

Draft
copybara-service wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: DSLX C++ transpiler must unique-ify generated temp variable names to not collide with DSLX struct member field names #3741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions xls/dslx/cpp_transpiler/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ cc_library(
"//xls/dslx/bytecode:bytecode_interpreter",
"//xls/dslx/frontend:ast",
"//xls/dslx/type_system:type_info",
"@com_google_absl//absl/algorithm:container",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/status",
"@com_google_absl//absl/status:statusor",
Expand Down
2 changes: 1 addition & 1 deletion xls/dslx/cpp_transpiler/cpp_transpiler_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ struct MyStruct {
y: u15,
z: u8,
w: s63,
v: u1,
result: u1,
})";

auto import_data = CreateImportDataForTest();
Expand Down
73 changes: 49 additions & 24 deletions xls/dslx/cpp_transpiler/cpp_type_generator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include <utility>
#include <vector>

#include "absl/algorithm/container.h"
#include "absl/container/flat_hash_map.h"
#include "absl/status/status.h"
#include "absl/status/statusor.h"
Expand All @@ -44,6 +45,24 @@
namespace xls::dslx {
namespace {

constexpr std::string_view kUniqueVarNamePrefix = "result";

std::string UniqueVarName(std::vector<std::string>& user_names) {
std::string unique_name = std::string(kUniqueVarNamePrefix);
if (!absl::c_contains(user_names, unique_name)) {
return unique_name;
}
std::vector<std::string> existing_user_names = user_names;
absl::c_sort(existing_user_names);
// Appending a character avoids collision with previous names due to sorting.
for (int64_t i = 0; i < existing_user_names.size(); ++i) {
if (existing_user_names[i] == unique_name) {
unique_name += "_";
}
}
return unique_name;
}

absl::StatusOr<InterpValue> InterpretExpr(
ImportData* import_data, TypeInfo* type_info, Expr* expr,
const absl::flat_hash_map<std::string, InterpValue>& env) {
Expand Down Expand Up @@ -479,7 +498,8 @@ class StructCppTypeGenerator : public CppTypeGenerator {
names.push_back(SanitizeCppName(struct_def->GetMemberName(i)));
}
return names;
}()) {}
}()),
tmp_var_name_(UniqueVarName(cpp_member_names_)) {}
~StructCppTypeGenerator() override = default;

static absl::StatusOr<std::unique_ptr<StructCppTypeGenerator>> Create(
Expand Down Expand Up @@ -566,14 +586,14 @@ class StructCppTypeGenerator : public CppTypeGenerator {
"tuple of %d elements.\");",
struct_def_->size()));
pieces.push_back("}");
pieces.push_back(absl::StrFormat("%s result;", cpp_type()));
pieces.push_back(absl::StrFormat("%s %s;", cpp_type(), tmp_var_name_));
for (int i = 0; i < struct_def_->members().size(); i++) {
std::string assignment = member_emitters_[i]->AssignFromValue(
/*lhs=*/absl::StrFormat("result.%s", cpp_member_names_[i]),
/*lhs=*/absl::StrFormat("%s.%s", tmp_var_name_, cpp_member_names_[i]),
/*rhs=*/absl::StrFormat("value.element(%d)", i), /*nesting=*/0);
pieces.push_back(assignment);
}
pieces.push_back("return result;");
pieces.push_back(absl::StrFormat("return %s;", tmp_var_name_));
std::string body = absl::StrJoin(pieces, "\n");

return CppSource{
Expand All @@ -588,16 +608,18 @@ class StructCppTypeGenerator : public CppTypeGenerator {

CppSource ToValueMethod() const {
std::vector<std::string> pieces;
pieces.push_back("std::vector<::xls::Value> members;");
pieces.push_back(
absl::StrFormat("members.resize(%d);", struct_def_->members().size()));
absl::StrFormat("std::vector<::xls::Value> %s;", tmp_var_name_));
pieces.push_back(absl::StrFormat("%s.resize(%d);", tmp_var_name_,
struct_def_->members().size()));
for (int i = 0; i < struct_def_->members().size(); i++) {
std::string assignment = member_emitters_[i]->AssignToValue(
/*lhs=*/absl::StrFormat("members[%d]", i),
/*lhs=*/absl::StrFormat("%s[%d]", tmp_var_name_, i),
/*rhs=*/cpp_member_names_[i], /*nesting=*/0);
pieces.push_back(assignment);
}
pieces.push_back("return ::xls::Value::Tuple(members);");
pieces.push_back(
absl::StrFormat("return ::xls::Value::Tuple(%s);", tmp_var_name_));
std::string body = absl::StrJoin(pieces, "\n");

return CppSource{
Expand Down Expand Up @@ -627,18 +649,18 @@ class StructCppTypeGenerator : public CppTypeGenerator {

CppSource ToStringMethod() const {
std::vector<std::string> pieces;
pieces.push_back(
absl::StrFormat("std::string result = \"%s {\\n\";", cpp_type()));
pieces.push_back(absl::StrFormat("std::string %s = \"%s {\\n\";",
tmp_var_name_, cpp_type()));
for (int i = 0; i < struct_def_->members().size(); i++) {
pieces.push_back(absl::StrFormat(
"result += __indent(indent + 1) + \"%s: \";", cpp_member_names_[i]));
pieces.push_back(absl::StrFormat("%s += __indent(indent + 1) + \"%s: \";",
tmp_var_name_, cpp_member_names_[i]));
std::string to_string = member_emitters_[i]->ToString(
"result", "indent + 2", cpp_member_names_[i], /*nesting=*/0);
tmp_var_name_, "indent + 2", cpp_member_names_[i], /*nesting=*/0);
pieces.push_back(to_string);
pieces.push_back("result += \",\\n\";");
pieces.push_back(tmp_var_name_ + " += \",\\n\";");
}
pieces.push_back("result += __indent(indent) + \"}\";");
pieces.push_back("return result;");
pieces.push_back(tmp_var_name_ + " += __indent(indent) + \"}\";");
pieces.push_back(absl::StrFormat("return %s;", tmp_var_name_));
std::string body = absl::StrJoin(pieces, "\n");

return CppSource{.header = "std::string ToString(int indent = 0) const;",
Expand All @@ -649,18 +671,18 @@ class StructCppTypeGenerator : public CppTypeGenerator {

CppSource ToDslxStringMethod() const {
std::vector<std::string> pieces;
pieces.push_back(
absl::StrFormat("std::string result = \"%s {\\n\";", dslx_type()));
pieces.push_back(absl::StrFormat("std::string %s = \"%s {\\n\";",
tmp_var_name_, dslx_type()));
for (int i = 0; i < struct_def_->members().size(); i++) {
pieces.push_back(absl::StrFormat(
"result += __indent(indent + 1) + \"%s: \";", cpp_member_names_[i]));
pieces.push_back(absl::StrFormat("%s += __indent(indent + 1) + \"%s: \";",
tmp_var_name_, cpp_member_names_[i]));
std::string to_string = member_emitters_[i]->ToDslxString(
"result", "indent + 2", cpp_member_names_[i], /*nesting=*/0);
tmp_var_name_, "indent + 2", cpp_member_names_[i], /*nesting=*/0);
pieces.push_back(to_string);
pieces.push_back("result += \",\\n\";");
pieces.push_back(tmp_var_name_ + " += \",\\n\";");
}
pieces.push_back("result += __indent(indent) + \"}\";");
pieces.push_back("return result;");
pieces.push_back(tmp_var_name_ + " += __indent(indent) + \"}\";");
pieces.push_back(absl::StrFormat("return %s;", tmp_var_name_));
std::string body = absl::StrJoin(pieces, "\n");

return CppSource{
Expand Down Expand Up @@ -712,6 +734,9 @@ class StructCppTypeGenerator : public CppTypeGenerator {
const StructDef* struct_def_;
std::vector<std::unique_ptr<CppEmitter>> member_emitters_;
std::vector<std::string> cpp_member_names_;
// The name of temporary variables in generated code that do not collide with
// user defined member names.
std::string tmp_var_name_;
};

} // namespace
Expand Down
18 changes: 9 additions & 9 deletions xls/dslx/cpp_transpiler/testdata/ArrayOfTyperefs.cctxt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,17 +55,17 @@ absl::StatusOr<Foo> Foo::FromValue(const ::xls::Value& value) {
}

absl::StatusOr<::xls::Value> Foo::ToValue() const {
std::vector<::xls::Value> members;
members.resize(2);
std::vector<::xls::Value> result;
result.resize(2);
if (!FitsInNBitsUnsigned(a, 32)) {
return absl::InvalidArgumentError(absl::StrFormat("Unsigned value %#x does not fit in 32 bits", a));
}
members[0] = ::xls::Value(::xls::UBits(a, 32));
result[0] = ::xls::Value(::xls::UBits(a, 32));
if (!FitsInNBitsUnsigned(b, 64)) {
return absl::InvalidArgumentError(absl::StrFormat("Unsigned value %#x does not fit in 64 bits", b));
}
members[1] = ::xls::Value(::xls::UBits(b, 64));
return ::xls::Value::Tuple(members);
result[1] = ::xls::Value(::xls::UBits(b, 64));
return ::xls::Value::Tuple(result);
}

std::string Foo::ToString(int indent) const {
Expand Down Expand Up @@ -126,18 +126,18 @@ absl::StatusOr<Bar> Bar::FromValue(const ::xls::Value& value) {
}

absl::StatusOr<::xls::Value> Bar::ToValue() const {
std::vector<::xls::Value> members;
members.resize(1);
std::vector<::xls::Value> result;
result.resize(1);
{
std::vector<::xls::Value> elements;
for (int64_t i0 = 0; i0 < 2; ++i0) {
::xls::Value element;
XLS_ASSIGN_OR_RETURN(element, c[i0].ToValue());
elements.push_back(element);
}
members[0] = ::xls::Value::ArrayOrDie(elements);
result[0] = ::xls::Value::ArrayOrDie(elements);
}
return ::xls::Value::Tuple(members);
return ::xls::Value::Tuple(result);
}

std::string Bar::ToString(int indent) const {
Expand Down
12 changes: 6 additions & 6 deletions xls/dslx/cpp_transpiler/testdata/BasicArray.cctxt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ absl::StatusOr<MyStruct> MyStruct::FromValue(const ::xls::Value& value) {
}

absl::StatusOr<::xls::Value> MyStruct::ToValue() const {
std::vector<::xls::Value> members;
members.resize(3);
std::vector<::xls::Value> result;
result.resize(3);
{
std::vector<::xls::Value> elements;
for (int64_t i0 = 0; i0 < 32; ++i0) {
Expand All @@ -86,7 +86,7 @@ absl::StatusOr<::xls::Value> MyStruct::ToValue() const {
element = ::xls::Value(::xls::UBits(x[i0], 32));
elements.push_back(element);
}
members[0] = ::xls::Value::ArrayOrDie(elements);
result[0] = ::xls::Value::ArrayOrDie(elements);
}
{
std::vector<::xls::Value> elements;
Expand All @@ -98,7 +98,7 @@ absl::StatusOr<::xls::Value> MyStruct::ToValue() const {
element = ::xls::Value(::xls::SBits(y[i0], 7));
elements.push_back(element);
}
members[1] = ::xls::Value::ArrayOrDie(elements);
result[1] = ::xls::Value::ArrayOrDie(elements);
}
{
std::vector<::xls::Value> elements;
Expand All @@ -110,9 +110,9 @@ absl::StatusOr<::xls::Value> MyStruct::ToValue() const {
element = ::xls::Value(::xls::UBits(z[i0], 8));
elements.push_back(element);
}
members[2] = ::xls::Value::ArrayOrDie(elements);
result[2] = ::xls::Value::ArrayOrDie(elements);
}
return ::xls::Value::Tuple(members);
return ::xls::Value::Tuple(result);
}

std::string MyStruct::ToString(int indent) const {
Expand Down
Loading