diff --git a/xls/dslx/constexpr_evaluator.cc b/xls/dslx/constexpr_evaluator.cc index 2df109dafa..b602734f85 100644 --- a/xls/dslx/constexpr_evaluator.cc +++ b/xls/dslx/constexpr_evaluator.cc @@ -727,7 +727,7 @@ absl::StatusOr> MakeConstexprEnv( } // Collect all the freevars that are constexpr. - FreeVariables freevars = GetFreeVariables(node); + FreeVariables freevars = GetFreeVariablesByPos(node); VLOG(5) << "freevar count for `" << node->ToString() << "`: " << freevars.GetFreeVariableCount(); freevars = freevars.DropBuiltinDefs(); diff --git a/xls/dslx/frontend/ast.cc b/xls/dslx/frontend/ast.cc index 518f4feeac..539e6aee79 100644 --- a/xls/dslx/frontend/ast.cc +++ b/xls/dslx/frontend/ast.cc @@ -339,6 +339,13 @@ absl::StatusOr ToTypeDefinition(AstNode* node) { node->GetNodeTypeName(), node->ToString())); } +const Span& FreeVariables::GetFirstNameRefSpan( + std::string_view identifier) const { + std::vector name_refs = values_.at(identifier); + CHECK(!name_refs.empty()); + return name_refs.at(0)->span(); +} + FreeVariables FreeVariables::DropBuiltinDefs() const { FreeVariables result; for (const auto& [identifier, name_refs] : values_) { @@ -401,28 +408,33 @@ absl::flat_hash_set FreeVariables::Keys() const { return result; } -FreeVariables GetFreeVariables(const AstNode* node, const Pos* start_pos) { +FreeVariables GetFreeVariablesByLambda( + const AstNode* node, + const std::function& consider_free) { DfsIteratorNoTypes it(node); FreeVariables freevars; while (it.HasNext()) { const AstNode* n = it.Next(); if (const auto* name_ref = dynamic_cast(n)) { - // If a start position was given we test whether the name definition - // occurs before that start position. (If none was given we accept all - // name refs.) - if (start_pos == nullptr) { + if (consider_free == nullptr || consider_free(*name_ref)) { freevars.Add(name_ref->identifier(), name_ref); - } else { - std::optional name_def_start = name_ref->GetNameDefStart(); - if (!name_def_start.has_value() || *name_def_start < *start_pos) { - freevars.Add(name_ref->identifier(), name_ref); - } } } } return freevars; } +FreeVariables GetFreeVariablesByPos(const AstNode* node, const Pos* start_pos) { + std::function consider_free = nullptr; + if (start_pos != nullptr) { + consider_free = [start_pos](const NameRef& name_ref) { + std::optional name_def_start = name_ref.GetNameDefStart(); + return !name_def_start.has_value() || name_def_start.value() < *start_pos; + }; + } + return GetFreeVariablesByLambda(node, consider_free); +} + std::string BuiltinTypeToString(BuiltinType t) { switch (t) { #define CASE(__enum, B, __str, ...) \ diff --git a/xls/dslx/frontend/ast.h b/xls/dslx/frontend/ast.h index ba28e5ab6c..28049375cc 100644 --- a/xls/dslx/frontend/ast.h +++ b/xls/dslx/frontend/ast.h @@ -213,10 +213,21 @@ class FreeVariables { // references, but the number of free variables). int64_t GetFreeVariableCount() const { return values_.size(); } + // Returns the span of the first `NameRef` that is referring to `identifier` + // in this free variables set. + const Span& GetFirstNameRefSpan(std::string_view identifier) const; + private: absl::flat_hash_map> values_; }; +// Generalized form of `GetFreeVariablesByPos()` below -- takes a lambda that +// helps us determine if a `NameRef` present in the `node` should be considered +// free. +FreeVariables GetFreeVariablesByLambda( + const AstNode* node, + const std::function& consider_free = nullptr); + // Retrieves all the free variables (references to names that are defined // prior to start_pos) that are transitively in this AST subtree. // @@ -225,10 +236,18 @@ class FreeVariables { // const FOO = u32:42; // fn main(x: u32) { FOO+x } // -// And using the starting point of the function as the start_pos, the FOO will -// be flagged as a free variable and returned. -FreeVariables GetFreeVariables(const AstNode* node, - const Pos* start_pos = nullptr); +// And *using the starting point of the function* as the `start_pos`, the `FOO` +// will be flagged as a free variable and returned. +// +// Note: the start_pos given is a way to approximate "free variable with +// respect to this AST construct". i.e. all the references with defs that are +// defined before this start_pos point are considered free. This gives an easy +// way to say "everything defined inside the body we don't need to worry about +// -- only tell me about references to things before this lexical position in +// the file" -- "lexical position in the file" is an approximation for +// "everything defined outside of (before) this AST construct". +FreeVariables GetFreeVariablesByPos(const AstNode* node, + const Pos* start_pos = nullptr); // Analogous to ToAstNode(), but for Expr base. template @@ -1708,6 +1727,14 @@ class Function : public AstNode { void set_proc(Proc* proc) { proc_ = proc; } bool IsInProc() const { return proc_.has_value(); } + std::optional GetParametricBindingsSpan() const { + if (parametric_bindings_.empty()) { + return std::nullopt; + } + return Span(parametric_bindings_.front()->span().start(), + parametric_bindings_.back()->span().limit()); + } + private: Span span_; NameDef* name_def_; @@ -2279,6 +2306,14 @@ class StructDef : public AstNode { return extern_type_name_; } + std::optional GetParametricBindingsSpan() const { + if (parametric_bindings_.empty()) { + return std::nullopt; + } + return Span(parametric_bindings_.front()->span().start(), + parametric_bindings_.back()->span().limit()); + } + private: Span span_; NameDef* name_def_; diff --git a/xls/dslx/frontend/parser_test.cc b/xls/dslx/frontend/parser_test.cc index 36d4076f9e..c8cf6dd3cb 100644 --- a/xls/dslx/frontend/parser_test.cc +++ b/xls/dslx/frontend/parser_test.cc @@ -1527,7 +1527,7 @@ TEST_F(ParserTest, MatchFreevars) { y => z, })", {"x", "y", "z"}); - FreeVariables fv = GetFreeVariables(e, &e->span().start()); + FreeVariables fv = GetFreeVariablesByPos(e, &e->span().start()); EXPECT_THAT(fv.Keys(), testing::ContainerEq( absl::flat_hash_set{"x", "y", "z"})); } @@ -1538,7 +1538,7 @@ TEST_F(ParserTest, ForFreevars) { new_accum }(u32:0))", {"range", "j"}); - FreeVariables fv = GetFreeVariables(e, &e->span().start()); + FreeVariables fv = GetFreeVariablesByPos(e, &e->span().start()); EXPECT_THAT(fv.Keys(), testing::ContainerEq( absl::flat_hash_set{"j", "range"})); } diff --git a/xls/dslx/ir_convert/function_converter.cc b/xls/dslx/ir_convert/function_converter.cc index f63e19898d..a2d32c7cc9 100644 --- a/xls/dslx/ir_convert/function_converter.cc +++ b/xls/dslx/ir_convert/function_converter.cc @@ -1282,7 +1282,7 @@ absl::Status FunctionConverter::HandleFor(const For* node) { // So we suffix free variables for the function body onto the function // parameters. FreeVariables freevars = - GetFreeVariables(node->body(), &node->span().start()); + GetFreeVariablesByPos(node->body(), &node->span().start()); freevars = freevars.DropBuiltinDefs(); std::vector relevant_name_defs; for (const auto& any_name_def : freevars.GetNameDefs()) { @@ -3505,7 +3505,7 @@ absl::StatusOr InterpValueToValue(const InterpValue& iv) { absl::StatusOr> GetConstantDepFreevars( AstNode* node) { Span span = node->GetSpan().value(); - FreeVariables free_variables = GetFreeVariables(node, &span.start()); + FreeVariables free_variables = GetFreeVariablesByPos(node, &span.start()); std::vector> freevars = free_variables.GetNameDefTuples(); std::vector constant_deps; diff --git a/xls/dslx/tests/errors/error_modules_test.py b/xls/dslx/tests/errors/error_modules_test.py index 59f393f9e1..b708e19c68 100644 --- a/xls/dslx/tests/errors/error_modules_test.py +++ b/xls/dslx/tests/errors/error_modules_test.py @@ -1181,6 +1181,13 @@ def test_width_slice_of_non_type_size(self): 'Expected type-reference to refer to a type definition', stderr ) + def test_gh_1473(self): + stderr = self._run('xls/dslx/tests/errors/gh_1473.x') + self.assertIn( + 'Parametric expression `umax(MAX_N_M, V)` refered to `V`' + ' which is not present in the parametric environment', stderr + ) + if __name__ == '__main__': test_base.main() diff --git a/xls/dslx/tests/errors/gh_1473.x b/xls/dslx/tests/errors/gh_1473.x new file mode 100644 index 0000000000..bb9e1d3eaa --- /dev/null +++ b/xls/dslx/tests/errors/gh_1473.x @@ -0,0 +1,41 @@ +// Copyright 2024 The XLS Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +import float32; + +pub fn umax(x: uN[N], y: uN[N]) -> uN[N] { if x > y { x } else { y } } + +pub fn uadd_with_overflow + + (x: uN[N], y: uN[M]) -> (bool, uN[V]) { + + let x_extended = widening_cast(x); + let y_extended = widening_cast(y); + + let full_result: uN[MAX_N_M_V + u32:1] = x_extended + y_extended; + let narrowed_result = full_result as uN[V]; + let overflow_detected = or_reduce(full_result[V as s32:]); + + (overflow_detected, narrowed_result) +} + +pub fn double_fraction_carry(f: float32::F32) -> (uN[float32::F32_FRACTION_SZ], u1) { + let f = f.fraction as uN[float32::F32_FRACTION_SZ + u32:1]; + let (overflow, f_x2) = uadd_with_overflow(f, f); + (f_x2, overflow) +} diff --git a/xls/dslx/type_system/parametric_instantiator_internal.cc b/xls/dslx/type_system/parametric_instantiator_internal.cc index dff3cb5d61..ddee202715 100644 --- a/xls/dslx/type_system/parametric_instantiator_internal.cc +++ b/xls/dslx/type_system/parametric_instantiator_internal.cc @@ -144,7 +144,15 @@ absl::Status EagerlyPopulateParametricEnvMap( absl::Span typed_parametrics, const absl::flat_hash_map& parametric_default_exprs, absl::flat_hash_map& parametric_env_map, - const Span& span, std::string_view kind_name, DeduceCtx* ctx) { + const std::optional& parametrics_span, const Span& span, + std::string_view kind_name, DeduceCtx* ctx) { + // If there are no parametric bindings being instantiated (in the callee) we + // should have already typechecked that there are no parametrics being + // applied by the caller. + if (!parametrics_span.has_value()) { + XLS_RET_CHECK(typed_parametrics.empty()); + } + // Attempt to interpret the parametric "default expressions" in order. for (const ParametricWithType& typed_parametric : typed_parametrics) { std::string_view name = typed_parametric.identifier(); @@ -180,6 +188,38 @@ absl::Status EagerlyPopulateParametricEnvMap( VLOG(5) << absl::StreamFormat("Evaluating expr: `%s` in env: %s", expr->ToString(), env.ToString()); + // If the expression requires variables that were not bound in the + // environment yet, we give an error message. + FreeVariables freevars = + GetFreeVariablesByLambda(expr, [&](const NameRef& name_ref) { + // If the name def is in the parametrics span, we consider it a + // freevar. + AnyNameDef any_name_def = name_ref.name_def(); + if (!std::holds_alternative(any_name_def)) { + return false; + } + const NameDef* name_def = std::get(any_name_def); + Span definition_span = name_def->GetSpan().value(); + if (definition_span.filename() != parametrics_span->filename()) { + return false; + } + return parametrics_span->Contains(definition_span); + }); + + auto keys_unsorted = freevars.Keys(); + absl::btree_set keys_sorted(keys_unsorted.begin(), + keys_unsorted.end()); + for (const std::string& key : keys_sorted) { + if (!parametric_env_map.contains(key)) { + return TypeInferenceErrorStatus( + freevars.GetFirstNameRefSpan(key), nullptr, + absl::StrFormat( + "Parametric expression `%s` refered to `%s` which is not " + "present in the parametric environment; instantiated from %s", + expr->ToString(), key, span.ToString())); + } + } + absl::StatusOr result = InterpretExpr(ctx, expr, env); VLOG(5) << "Interpreted expr: " << expr->ToString() << " @ " << expr->span() @@ -209,18 +249,36 @@ absl::Status EagerlyPopulateParametricEnvMap( parametric_env_map.insert({std::string{name}, result.value()}); } } + + // TODO(https://github.com/google/xls/issues/1495): 2024-06-18 We would like + // to enable this invariant to tighten up what is accepted by the type + // system, but that requires some investigation into failing samples. + if (false) { + // Check that all parametric bindings are present in the env. + for (const auto& [parametric_binding_name, _] : parametric_default_exprs) { + if (!parametric_env_map.contains(parametric_binding_name)) { + return TypeInferenceErrorStatus( + span, nullptr, + absl::StrFormat("Caller did not supply parametric value for `%s`", + parametric_binding_name)); + } + } + } + return absl::OkStatus(); } } // namespace ParametricInstantiator::ParametricInstantiator( - Span span, absl::Span args, DeduceCtx* ctx, + Span span, std::optional parametrics_span, + absl::Span args, DeduceCtx* ctx, absl::Span typed_parametrics, const absl::flat_hash_map& explicit_parametrics, absl::Span const> parametric_bindings) : span_(std::move(span)), + parametrics_span_(std::move(parametrics_span)), args_(args), ctx_(ABSL_DIE_IF_NULL(ctx)), typed_parametrics_(typed_parametrics), @@ -339,7 +397,7 @@ absl::StatusOr FunctionInstantiator::Instantiate() { XLS_RETURN_IF_ERROR(EagerlyPopulateParametricEnvMap( typed_parametrics(), parametric_default_exprs(), parametric_env_map(), - span(), GetKindName(), &ctx())); + parametrics_span(), span(), GetKindName(), &ctx())); // Phase 2: resolve and check. VLOG(10) << "Phase 2: resolve-and-check"; @@ -404,7 +462,7 @@ absl::StatusOr StructInstantiator::Instantiate() { XLS_RETURN_IF_ERROR(EagerlyPopulateParametricEnvMap( typed_parametrics(), parametric_default_exprs(), parametric_env_map(), - span(), GetKindName(), &ctx())); + parametrics_span(), span(), GetKindName(), &ctx())); // Phase 2: resolve and check. for (int64_t i = 0; i < member_types_.size(); ++i) { diff --git a/xls/dslx/type_system/parametric_instantiator_internal.h b/xls/dslx/type_system/parametric_instantiator_internal.h index 58f3c1859c..51ba3b53a5 100644 --- a/xls/dslx/type_system/parametric_instantiator_internal.h +++ b/xls/dslx/type_system/parametric_instantiator_internal.h @@ -64,7 +64,8 @@ class ParametricInstantiator { // typed_parametrics/explicit_parametrics and member comments for other // arguments. ParametricInstantiator( - Span span, absl::Span args, DeduceCtx* ctx, + Span span, std::optional parametrics_span, + absl::Span args, DeduceCtx* ctx, absl::Span typed_parametrics, const absl::flat_hash_map& explicit_parametrics, absl::Span const> @@ -108,6 +109,18 @@ class ParametricInstantiator { return typed_parametrics_; } + // Returns the span of the parametrics; e.g. + // + // f(...) + // ^~~~~~~~~~~~~~~^ this + // + // Nullopt when we're (presumably uselessly) instantiating a function that + // has no parametrics, which does seem to happen in the type system at + // present. + const std::optional& parametrics_span() const { + return parametrics_span_; + } + const Span& span() const { return span_; } DeduceCtx& ctx() { return *ctx_; } @@ -116,6 +129,9 @@ class ParametricInstantiator { // instantiated. Span span_; + // Span of parametric bindings being instantiated; see `parametrics_span()`. + std::optional parametrics_span_; + // Arguments driving the instantiation, see `InstantiateArg` for more details. absl::Span args_; @@ -171,8 +187,9 @@ class FunctionInstantiator : public ParametricInstantiator { const absl::flat_hash_map& explicit_parametrics, absl::Span const> parametric_bindings) - : ParametricInstantiator(std::move(span), args, ctx, typed_parametrics, - explicit_parametrics, parametric_bindings), + : ParametricInstantiator( + std::move(span), callee_fn.GetParametricBindingsSpan(), args, ctx, + typed_parametrics, explicit_parametrics, parametric_bindings), callee_fn_(callee_fn), function_type_(CloneToUnique(function_type)), param_types_(function_type_->params()) {} @@ -205,10 +222,11 @@ class StructInstantiator : public ParametricInstantiator { absl::Span typed_parametrics, absl::Span const> parametric_bindings) - : ParametricInstantiator(std::move(span), args, ctx, - /*typed_parametrics=*/typed_parametrics, - /*explicit_parametrics=*/{}, - parametric_bindings), + : ParametricInstantiator( + std::move(span), + struct_type.nominal_type().GetParametricBindingsSpan(), args, ctx, + /*typed_parametrics=*/typed_parametrics, + /*explicit_parametrics=*/{}, parametric_bindings), struct_type_(CloneToUnique(struct_type)), member_types_(member_types) {} diff --git a/xls/dslx/type_system/typecheck_module_test.cc b/xls/dslx/type_system/typecheck_module_test.cc index 9a59520b44..55c044c949 100644 --- a/xls/dslx/type_system/typecheck_module_test.cc +++ b/xls/dslx/type_system/typecheck_module_test.cc @@ -315,6 +315,57 @@ fn f() -> bool { p < u32:42 } "not being invoked"))); } +// X is not bound in this example but it's also not used anywhere -- currently +// this is accepted. +// +// TODO(https://github.com/google/xls/issues/1495): We'd like this to be an +// error in the future. +TEST(TypecheckTest, Gh1473_UnboundButAlsoUnusedParametricNoDefaultExpr) { + std::string program = R"( +fn p(y: uN[Y]) -> u32 { Y } +fn f() -> u32 { p(u7:0) } +)"; + XLS_EXPECT_OK(Typecheck(program)); +} + +TEST(TypecheckErrorTest, Gh1473_UnboundButAlsoUnusedParametricWithDefaultExpr) { + std::string program = R"( +fn p(y: uN[Y]) -> u32 { Y } +fn f() -> u32 { p(u7:0) } +)"; + EXPECT_THAT( + Typecheck(program), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("Parametric expression `X + X` refered to `X` which " + "is not present in the parametric environment"))); +} + +// In this example we do not bind X via the arguments, but we try to use it in +// forming a return type. +TEST(TypecheckErrorTest, Gh1473_UnboundAndUsedParametric) { + std::string program = R"( +fn p(y: uN[Y]) -> uN[X] { Y } +fn f() -> u32 { p(u7:0) } +)"; + EXPECT_THAT(Typecheck(program), + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("uN[X] Instantiated return type did not have " + "all parametrics resolved"))); +} + +// In this example we do not bind X via the arguments, but we try to use it in +// the body of the function. +// +// TODO(https://github.com/google/xls/issues/1495): This surprisingly works in +// type checking but fails when we try to IR convert it, we should fix that. +TEST(TypecheckTest, Gh1473_UnboundAndUsedParametricInBody) { + std::string program = R"( +fn p(y: uN[Y]) -> bool { uN[X]:0 == uN[X]:1 } +fn f() -> bool { p(u7:0) } +)"; + XLS_EXPECT_OK(Typecheck(program)); +} + TEST(TypecheckTest, MapOfParametric) { std::string program = R"( fn p(x: bits[N]) -> bits[N] { x }