Skip to content

Commit

Permalink
Revert "Fix infinite loop in type checking with inverted parametric n…
Browse files Browse the repository at this point in the history 
…ame order."

This reverts commit 9081e20.
  • Loading branch information
@lpawelcz
lpawelcz committed Oct 9, 2024
1 parent 1222420 commit 85f1f80
Show file tree
Hide file tree
Showing 9 changed files with 91 additions and 224 deletions.
27 changes: 1 addition & 26 deletions xls/dslx/ir_convert/ir_converter_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -887,10 +887,9 @@ fn f(outer_thing_1: u32, outer_thing_2: u32) -> u32 {

TEST(IrConverterTest, ParametricDefaultInStruct) {
const char* kProgram = R"(
struct Foo <X: u32, Y: u32 = {X + u32:1}, Z: u32 = {Y + u32:1}> {
struct Foo <X: u32, Y: u32 = {X + u32:1}> {
a: uN[X],
b: uN[Y],
c: uN[Z]
}
fn make_zero_foo<X: u32>() -> Foo<X> {
Expand All @@ -908,30 +907,6 @@ fn test() -> Foo<u32:5> {
ExpectIr(converted, TestName());
}

// See https://github.com/google/xls/issues/1615
TEST(IrConverterTest, ParametricStructReverseOrderParametrics) {
const char* kProgram = R"(
struct Foo<X: u32, Y: u32, Z:u32 = {X}> {
a: uN[X],
b: uN[Y],
c: uN[Z],
}
fn make_zero_foo<X: u32, Y: u32>() -> Foo<Y, X> {
Foo<Y, X> { a: zero!<uN[Y]>(), b: zero!<uN[X]>(), c: zero!<uN[Y]>() }
}
fn test() -> Foo<u32:6, u32:5> {
make_zero_foo<u32:5, u32:6>()
}
)";

XLS_ASSERT_OK_AND_ASSIGN(
std::string converted,
ConvertModuleForTest(kProgram, ConvertOptions{.emit_positions = false}));
ExpectIr(converted, TestName());
}

TEST(IrConverterTest, ParametricDefaultClog2InStruct) {
const char* kProgram = R"(
import std;
Expand Down
8 changes: 4 additions & 4 deletions xls/dslx/ir_convert/testdata/ir_converter_test_ParametricDefaultInStruct.ir
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@ package test_module

file_number 0 "test_module.x"

fn __test_module__make_zero_foo__5() -> (bits[5], bits[6], bits[7]) {
fn __test_module__make_zero_foo__5() -> (bits[5], bits[6]) {
X: bits[32] = literal(value=5, id=1)
ret literal.2: (bits[5], bits[6], bits[7]) = literal(value=(0, 0, 0), id=2)
ret literal.2: (bits[5], bits[6]) = literal(value=(0, 0), id=2)
}

fn __test_module__test() -> (bits[5], bits[6], bits[7]) {
ret invoke.3: (bits[5], bits[6], bits[7]) = invoke(to_apply=__test_module__make_zero_foo__5, id=3)
fn __test_module__test() -> (bits[5], bits[6]) {
ret invoke.3: (bits[5], bits[6]) = invoke(to_apply=__test_module__make_zero_foo__5, id=3)
}
16 changes: 0 additions & 16 deletions xls/dslx/ir_convert/testdata/ir_converter_test_ParametricStructReverseOrderParametrics.ir
View file

This file was deleted.

48 changes: 38 additions & 10 deletions xls/dslx/type_system/deduce.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,29 @@ absl::StatusOr<InterpValue> EvaluateConstexprValue(DeduceCtx* ctx,
ctx->GetCurrentParametricEnv(), node, type);
}

// Evaluates the given `dim` to the extent possible with the given `env`.
// Examples:
// - If `dim` is already a constant, the result is `dim`.
// - If `dim` is `X + 1`, and `env` says `X` == `5`, then the result is `6`.
// - If `dim` is `X + 1`, and `env` says `X` == `Y` and does not have `Y`,
// then the result is `Y + 1`.
// - If `dim` is `X + 1`, and `env` says `X` == `Y` and `Y` == `5`, then the
// result is `6`.
TypeDim EvaluateTypeDim(TypeDim dim, const ParametricExpression::Env& env) {
absl::flat_hash_set<std::string> prev_free_variables;
while (std::holds_alternative<TypeDim::OwnedParametric>(dim.value())) {
auto& parametric = std::get<TypeDim::OwnedParametric>(dim.value());
absl::flat_hash_set<std::string> next_free_variables =
parametric->GetFreeVariables();
if (prev_free_variables == next_free_variables) {
break;
}
prev_free_variables = std::move(next_free_variables);
dim = TypeDim(parametric->Evaluate(env));
}
return dim;
}

// Attempts to convert an expression from the full DSL AST into the
// ParametricExpression sub-AST (a limited form that we can embed into a
// TypeDim for later instantiation).
Expand Down Expand Up @@ -1855,7 +1878,7 @@ static absl::StatusOr<std::unique_ptr<Type>> ConcretizeStructAnnotation(
ctx->file_table());
}

TypeDimMap type_dim_map;
absl::flat_hash_map<std::string, TypeDim> parametric_env;
for (int64_t i = 0; i < type_annotation->parametrics().size(); ++i) {
ParametricBinding* defined_parametric =
struct_def->parametric_bindings()[i];
Expand All @@ -1866,7 +1889,7 @@ static absl::StatusOr<std::unique_ptr<Type>> ConcretizeStructAnnotation(

XLS_ASSIGN_OR_RETURN(TypeDim ctd,
DimToConcreteUsize(annotated_parametric, ctx));
type_dim_map.Insert(defined_parametric->identifier(), std::move(ctd));
parametric_env.emplace(defined_parametric->identifier(), std::move(ctd));
}

// For the remainder of the formal parameterics (i.e. after the explicitly
Expand All @@ -1887,27 +1910,32 @@ static absl::StatusOr<std::unique_ptr<Type>> ConcretizeStructAnnotation(
}
XLS_ASSIGN_OR_RETURN(std::unique_ptr<ParametricExpression> parametric_expr,
ExprToParametric(defined_parametric->expr(), ctx));
type_dim_map.Insert(defined_parametric->identifier(),
TypeDim(parametric_expr->Evaluate(type_dim_map.env())));
parametric_env.emplace(defined_parametric->identifier(),
TypeDim(std::move(parametric_expr)));
}

ParametricExpression::Env env;
for (const auto& [k, ctd] : parametric_env) {
if (std::holds_alternative<InterpValue>(ctd.value())) {
env[k] = std::get<InterpValue>(ctd.value());
} else {
env[k] = &ctd.parametric();
}
}

// Now evaluate all the dimensions according to the values we've got.
XLS_ASSIGN_OR_RETURN(
std::unique_ptr<Type> mapped_type,
base_type.MapSize([&](const TypeDim& dim) -> absl::StatusOr<TypeDim> {
if (std::holds_alternative<TypeDim::OwnedParametric>(dim.value())) {
auto& parametric = std::get<TypeDim::OwnedParametric>(dim.value());
return TypeDim(parametric->Evaluate(type_dim_map.env()));
}
return dim;
return EvaluateTypeDim(dim, env);
}));

VLOG(5) << "ConcreteStructAnnotation mapped type: "
<< mapped_type->ToString();

// Attach the nominal parametrics to the type, so that we will remember the
// fact that we have instantiated e.g. Foo<M:u32, N:u32> as Foo<5, 6>.
return mapped_type->AddNominalTypeDims(type_dim_map.dims());
return mapped_type->AddNominalTypeDims(parametric_env);
}

absl::StatusOr<std::unique_ptr<Type>> DeduceTypeRefTypeAnnotation(
Expand Down
13 changes: 8 additions & 5 deletions xls/dslx/type_system/parametric_instantiator_internal.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,17 @@ absl::StatusOr<std::unique_ptr<Type>> ResolveInternal(
return TypeDim(std::move(evaluated));
}));
if (!parametric_env_map.empty()) {
// We need to resolve nominal dims upon parametric instantiation, in order
// for e.g. the return type of `bar` to get 8 rather than a meaningless `N`
// as the nominal dim in:
// We need to add nominal dims upon parametric instantiation, in order for
// e.g. the return type of `bar` to get 8 rather than a meaningless `N` as
// the nominal dim in:
// `struct S<N:u32> { ... }
// fn foo<N: u32>() -> S<N> { ... }
// fn bar() -> S<u32:8> { foo<u32:8>() }`
resolved =
resolved->ResolveNominalTypeDims(TypeDimMap(parametric_env_map).env());
absl::flat_hash_map<std::string, TypeDim> nominal_dims;
for (const auto& [key, interp_value] : parametric_env_map) {
nominal_dims.emplace(key, TypeDim(interp_value));
}
resolved = resolved->AddNominalTypeDims(nominal_dims);
}
VLOG(5) << "Resolved " << annotated.ToString() << " to "
<< resolved->ToString();
Expand Down
82 changes: 26 additions & 56 deletions xls/dslx/type_system/type.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,26 +49,6 @@
#include "xls/ir/bits_ops.h"

namespace xls::dslx {
namespace {

ParametricExpression::EnvValue TypeDimToEnvValue(const TypeDim& dim) {
if (std::holds_alternative<InterpValue>(dim.value())) {
return std::get<InterpValue>(dim.value());
}
return ParametricExpression::EnvValue(&dim.parametric());
}

std::vector<std::unique_ptr<Type>> CloneStructMembers(
const std::vector<std::unique_ptr<Type>>& members) {
std::vector<std::unique_ptr<Type>> cloned_members;
cloned_members.reserve(members.size());
for (auto& next : members) {
cloned_members.push_back(next->CloneToUnique());
}
return cloned_members;
}

} // namespace

Type::~Type() = default;

Expand Down Expand Up @@ -307,21 +287,6 @@ absl::StatusOr<bool> TypeDim::GetAsBool() const {
std::get<OwnedParametric>(value_)->ToString()));
}

// -- TypeDimMap

void TypeDimMap::Insert(std::string_view identifier, TypeDim dim) {
const TypeDim& stored_dim =
dims_.insert_or_assign(identifier, std::move(dim)).first->second;
env_.insert_or_assign(identifier, TypeDimToEnvValue(stored_dim));
}

TypeDimMap::TypeDimMap(
const absl::flat_hash_map<std::string, InterpValue>& values) {
for (const auto& [key, value] : values) {
Insert(key, TypeDim(value));
}
}

// -- Type

bool Type::CompatibleWith(const Type& other) const {
Expand Down Expand Up @@ -613,31 +578,36 @@ std::vector<TypeDim> StructType::GetAllDims() const {
}

std::unique_ptr<Type> StructType::AddNominalTypeDims(
const absl::flat_hash_map<std::string, TypeDim>& dims) const {
CHECK(nominal_type_dims_by_identifier_.empty());
absl::flat_hash_map<std::string, TypeDim> new_dims;
const absl::flat_hash_map<std::string, TypeDim>& added_dims_by_identifier)
const {
absl::flat_hash_map<std::string, TypeDim> combined_dims =
nominal_type_dims_by_identifier_;
for (const ParametricBinding* binding : struct_def_.parametric_bindings()) {
const auto it = dims.find(binding->identifier());
if (it != dims.end()) {
new_dims.emplace(binding->identifier(), it->second);
const auto existing_it = combined_dims.find(binding->identifier());
// Don't overwrite a dim that already has a concrete value, because that
// could lead to the parametric instantiator mis-attributing the `X` in
// `foo` to the unrelated `X` in `Bar` for something like:
// `struct Bar<X:u32> { ... }
// fn foo<X:u32> { Bar<u32:8>{...} }`
// Really the instantiator should eventually be re-factored to not even come
// close to doing this.
if (existing_it != combined_dims.end() &&
!std::holds_alternative<TypeDim::OwnedParametric>(
existing_it->second.value())) {
continue;
}
}
return std::make_unique<StructType>(CloneStructMembers(members_), struct_def_,
std::move(new_dims));
}

std::unique_ptr<Type> StructType::ResolveNominalTypeDims(
const ParametricExpression::Env& env) const {
absl::flat_hash_map<std::string, TypeDim> new_dims =
nominal_type_dims_by_identifier_;
for (auto& [key, dim] : new_dims) {
if (std::holds_alternative<TypeDim::OwnedParametric>(dim.value())) {
dim = TypeDim(
std::get<TypeDim::OwnedParametric>(dim.value())->Evaluate(env));
const auto it = added_dims_by_identifier.find(binding->identifier());
if (it != added_dims_by_identifier.end()) {
combined_dims.insert_or_assign(binding->identifier(), it->second.Clone());
}
}
return std::make_unique<StructType>(CloneStructMembers(members_), struct_def_,
std::move(new_dims));
std::vector<std::unique_ptr<Type>> cloned_members;
cloned_members.reserve(members_.size());
for (auto& next : members_) {
cloned_members.push_back(next->CloneToUnique());
}
return std::make_unique<StructType>(std::move(cloned_members), struct_def_,
std::move(combined_dims));
}

absl::StatusOr<TypeDim> StructType::GetTotalBitCount() const {
Expand Down
62 changes: 6 additions & 56 deletions xls/dslx/type_system/type.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,44 +129,6 @@ class TypeDim {
std::variant<InterpValue, OwnedParametric> value_;
};

// A utility for building a `ParametricExpression::Env` using `TypeDim` objects.
// Because a `ParametricExpression::Env` cannot own the `ParametricExpression`
// objects in it, the `TypeDimMap` also contains a backing store which owns
// those.
class TypeDimMap {
public:
TypeDimMap() = default;

// For the use case where the caller is starting with a map of known concrete
// values, or has nothing but that. Equivalent to constructing and then
// `Insert()`ing them all.
explicit TypeDimMap(
const absl::flat_hash_map<std::string, InterpValue>& values);

// Disallow copy; if we wanted this to work, we'd need to fix up `env_` in the
// copy to point to its own `dims_`.
TypeDimMap(const TypeDimMap&) = delete;
TypeDimMap& operator=(const TypeDimMap&) = delete;

// Inserts a `dim` that has the given `identifier` (according to e.g. the
// parametric bindings of a struct definition).
void Insert(std::string_view identifier, TypeDim dim);

// Returns an environment that can be used to evaluate a
// `ParametricExpression` against the values in this map. The returned object
// is only valid during the lifetime of this `TypeDimMap`.
const ParametricExpression::Env& env() const { return env_; }

// Returns a direct view of the dims that have been inserted.
const absl::flat_hash_map<std::string, TypeDim>& dims() const {
return dims_;
}

private:
absl::flat_hash_map<std::string, TypeDim> dims_;
ParametricExpression::Env env_;
};

inline std::ostream& operator<<(std::ostream& os, const TypeDim& ctd) {
os << ctd.ToString();
return os;
Expand Down Expand Up @@ -373,13 +335,6 @@ class Type {
return CloneToUnique();
}

// Returns a clone of this type that has any previously added nominal type
// dims as resolved as possible using the given environment.
virtual std::unique_ptr<Type> ResolveNominalTypeDims(
const ParametricExpression::Env&) const {
return CloneToUnique();
}

// Type equality, but ignores tuple member naming discrepancies.
bool CompatibleWith(const Type& other) const;

Expand Down Expand Up @@ -447,13 +402,10 @@ class MetaType : public Type {
}

std::unique_ptr<Type> AddNominalTypeDims(
const absl::flat_hash_map<std::string, TypeDim>& dims) const override {
return std::make_unique<MetaType>(wrapped_->AddNominalTypeDims(dims));
}

std::unique_ptr<Type> ResolveNominalTypeDims(
const ParametricExpression::Env& env) const override {
return std::make_unique<MetaType>(wrapped_->ResolveNominalTypeDims(env));
const absl::flat_hash_map<std::string, TypeDim>& dims_by_identifier)
const override {
return std::make_unique<MetaType>(
wrapped_->AddNominalTypeDims(dims_by_identifier));
}

std::unique_ptr<Type> CloneToUnique() const override {
Expand Down Expand Up @@ -602,10 +554,8 @@ class StructType : public Type {
const std::vector<std::unique_ptr<Type>>& members() const { return members_; }

std::unique_ptr<Type> AddNominalTypeDims(
const absl::flat_hash_map<std::string, TypeDim>& dims) const override;

std::unique_ptr<Type> ResolveNominalTypeDims(
const ParametricExpression::Env&) const override;
const absl::flat_hash_map<std::string, TypeDim>& dims_by_identifier)
const override;

private:
std::vector<std::unique_ptr<Type>> members_;
Expand Down
Loading

0 comments on commit 85f1f80

Please sign in to comment.