From 713c194f5872fdd979216d308e4a8fc2050ced3e Mon Sep 17 00:00:00 2001 From: Chris Leary Date: Thu, 28 Nov 2024 14:00:52 -0800 Subject: [PATCH 1/2] [DSLX:cleanup] Remove ConstantArray node from AST, it is misleading / not useful. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously ConstantArray was trying to convey some type-system-level properties as imbued in an AST node, but it was not fully accurate, and generally only served to confuse things. This gets us back to where the AST just represents the parsed syntax and the typechecker figures out constexpr properties and annotates them in the type information. This also allows us to seal up the Array AST node as final — subclassing of AST nodes can be awkward, we generally use hierarchy in the AST very sparingly. This reworks the deduce routine a bit to consolidate on the main discrepancy that was there between Arrays and ConstArrays, which was that we would, for user convenience, annotate the element type on any un-annotated numerical literals contained within the array. This will become better and more general will type inference 2.0 but until that comes we want to make sure we don’t have to change code in a sprawling way. Just trying to tighten up invariants in advance of somebody investigating google/xls#1748 --- xls/dslx/bytecode/bytecode_emitter.cc | 4 - xls/dslx/bytecode/bytecode_emitter.h | 1 - xls/dslx/constexpr_evaluator.cc | 5 - xls/dslx/constexpr_evaluator.h | 1 - xls/dslx/frontend/ast.cc | 15 +- xls/dslx/frontend/ast.h | 25 +- xls/dslx/frontend/ast_cloner.cc | 17 -- xls/dslx/frontend/ast_cloner_test.cc | 2 +- xls/dslx/frontend/ast_test.cc | 7 + xls/dslx/frontend/parser.cc | 4 - xls/dslx/frontend/parser_test.cc | 10 +- .../ir_convert/extract_conversion_order.cc | 4 - xls/dslx/ir_convert/function_converter.cc | 9 - xls/dslx/ir_convert/function_converter.h | 1 - xls/dslx/type_system/deduce.cc | 1 - xls/dslx/type_system/deduce_expr.cc | 233 +++++++++++------- xls/dslx/type_system/deduce_expr.h | 3 - xls/dslx/type_system/deduce_utils.cc | 12 + xls/dslx/type_system/deduce_utils.h | 8 +- xls/dslx/type_system/typecheck_module_test.cc | 17 +- xls/tools/proto_to_dslx.cc | 5 +- 21 files changed, 186 insertions(+), 198 deletions(-) diff --git a/xls/dslx/bytecode/bytecode_emitter.cc b/xls/dslx/bytecode/bytecode_emitter.cc index a4d103a47c..50fca1520d 100644 --- a/xls/dslx/bytecode/bytecode_emitter.cc +++ b/xls/dslx/bytecode/bytecode_emitter.cc @@ -881,10 +881,6 @@ absl::Status BytecodeEmitter::HandleConstAssert(const ConstAssert* node) { return absl::OkStatus(); } -absl::Status BytecodeEmitter::HandleConstantArray(const ConstantArray* node) { - return HandleArray(node); -} - absl::Status BytecodeEmitter::HandleConstRef(const ConstRef* node) { return HandleNameRef(node); } diff --git a/xls/dslx/bytecode/bytecode_emitter.h b/xls/dslx/bytecode/bytecode_emitter.h index fb1ad8aa9b..76ac6ef7d3 100644 --- a/xls/dslx/bytecode/bytecode_emitter.h +++ b/xls/dslx/bytecode/bytecode_emitter.h @@ -91,7 +91,6 @@ class BytecodeEmitter : public ExprVisitor { absl::Status HandleColonRef(const ColonRef* node) override; absl::StatusOr HandleColonRefInternal(const ColonRef* node); absl::Status HandleConstAssert(const ConstAssert* node) override; - absl::Status HandleConstantArray(const ConstantArray* node) override; absl::Status HandleConstRef(const ConstRef* node) override; absl::Status HandleFor(const For* node) override; absl::Status HandleFormatMacro(const FormatMacro* node) override; diff --git a/xls/dslx/constexpr_evaluator.cc b/xls/dslx/constexpr_evaluator.cc index 8585a6c5dd..a8684b41bf 100644 --- a/xls/dslx/constexpr_evaluator.cc +++ b/xls/dslx/constexpr_evaluator.cc @@ -397,11 +397,6 @@ absl::Status ConstexprEvaluator::HandleColonRef(const ColonRef* expr) { subject); } -absl::Status ConstexprEvaluator::HandleConstantArray( - const ConstantArray* expr) { - return HandleArray(expr); -} - absl::Status ConstexprEvaluator::HandleConstAssert( const ConstAssert* const_assert) { const FileTable& file_table = *const_assert->owner()->file_table(); diff --git a/xls/dslx/constexpr_evaluator.h b/xls/dslx/constexpr_evaluator.h index 8a4f7a106c..a58da9e2b8 100644 --- a/xls/dslx/constexpr_evaluator.h +++ b/xls/dslx/constexpr_evaluator.h @@ -73,7 +73,6 @@ class ConstexprEvaluator : public xls::dslx::ExprVisitor { absl::Status HandleChannelDecl(const ChannelDecl* expr) override; absl::Status HandleColonRef(const ColonRef* expr) override; absl::Status HandleConstAssert(const ConstAssert* const_assert) override; - absl::Status HandleConstantArray(const ConstantArray* expr) override; absl::Status HandleConstRef(const ConstRef* expr) override; absl::Status HandleFor(const For* expr) override; absl::Status HandleFunctionRef(const FunctionRef* expr) override; diff --git a/xls/dslx/frontend/ast.cc b/xls/dslx/frontend/ast.cc index 592b2abad3..f3a9fa0102 100644 --- a/xls/dslx/frontend/ast.cc +++ b/xls/dslx/frontend/ast.cc @@ -728,19 +728,6 @@ Array::Array(Module* owner, Span span, std::vector members, members_(std::move(members)), has_ellipsis_(has_ellipsis) {} -ConstantArray::ConstantArray(Module* owner, Span span, - std::vector members, bool has_ellipsis, - bool in_parens) - : Array(owner, std::move(span), std::move(members), has_ellipsis, - in_parens) { - for (Expr* expr : this->members()) { - CHECK(IsConstant(expr)) - << "non-constant in constant array: " << expr->ToString(); - } -} - -ConstantArray::~ConstantArray() = default; - // -- class TypeRef TypeRef::TypeRef(Module* owner, Span span, TypeDefinition type_definition) @@ -913,7 +900,7 @@ SelfTypeAnnotation::~SelfTypeAnnotation() = default; BuiltinNameDef::~BuiltinNameDef() = default; bool IsConstant(AstNode* node) { - if (IsOneOf(node)) { + if (IsOneOf(node)) { return true; } if (IsOneOf(node)) { diff --git a/xls/dslx/frontend/ast.h b/xls/dslx/frontend/ast.h index 40c6ead55d..3211a975a3 100644 --- a/xls/dslx/frontend/ast.h +++ b/xls/dslx/frontend/ast.h @@ -54,7 +54,6 @@ X(Cast) \ X(ChannelDecl) \ X(ColonRef) \ - X(ConstantArray) \ X(ConstRef) \ X(For) \ X(FormatMacro) \ @@ -1116,7 +1115,7 @@ class TypeAlias : public AstNode { }; // Represents an array expression; e.g. `[a, b, c]`. -class Array : public Expr { +class Array final : public Expr { public: Array(Module* owner, Span span, std::vector members, bool has_ellipsis, bool in_parens = false); @@ -1168,22 +1167,6 @@ class Array : public Expr { bool has_ellipsis_; }; -// A constant array expression is an array expression where all of the -// expressions contained within it are constant. -class ConstantArray : public Array { - public: - // Adds checking for constant-expression-ness of the members beyond - // Array::Array. - ConstantArray(Module* owner, Span span, std::vector members, - bool has_ellipsis, bool in_parens = false); - - ~ConstantArray() override; - - absl::Status Accept(AstNodeVisitor* v) const override { - return v->HandleConstantArray(this); - } -}; - // Several different AST nodes define types that can be referred to by a // TypeRef. using TypeDefinition = @@ -3406,10 +3389,8 @@ class VerbatimNode : public Expr { std::string text_; }; -// Helper for determining whether an AST node is constant (e.g., can be -// considered a constant value in a ConstantArray). In general a node -// with no children is considered constant, but there are some exceptions -// (e.g., NameRef). +// Helper for determining whether an AST node is constant (e.g., clearly can be +// considered a constant value before type checking). bool IsConstant(AstNode* n); } // namespace xls::dslx diff --git a/xls/dslx/frontend/ast_cloner.cc b/xls/dslx/frontend/ast_cloner.cc index 1377984abb..07d658c945 100644 --- a/xls/dslx/frontend/ast_cloner.cc +++ b/xls/dslx/frontend/ast_cloner.cc @@ -241,23 +241,6 @@ class AstCloner : public AstNodeVisitor { return absl::OkStatus(); } - absl::Status HandleConstantArray(const ConstantArray* n) override { - XLS_RETURN_IF_ERROR(VisitChildren(n)); - - std::vector new_members; - for (const Expr* old_member : n->members()) { - new_members.push_back(down_cast(old_to_new_.at(old_member))); - } - ConstantArray* new_array = module_->Make( - n->span(), new_members, n->has_ellipsis(), n->in_parens()); - if (n->type_annotation() != nullptr) { - new_array->set_type_annotation( - down_cast(old_to_new_.at(n->type_annotation()))); - } - old_to_new_[n] = new_array; - return absl::OkStatus(); - } - absl::Status HandleConstantDef(const ConstantDef* n) override { XLS_RETURN_IF_ERROR(VisitChildren(n)); TypeAnnotation* new_type_annotation = nullptr; diff --git a/xls/dslx/frontend/ast_cloner_test.cc b/xls/dslx/frontend/ast_cloner_test.cc index 0262671849..e753618544 100644 --- a/xls/dslx/frontend/ast_cloner_test.cc +++ b/xls/dslx/frontend/ast_cloner_test.cc @@ -655,7 +655,7 @@ fn main() -> foo::BAR { EXPECT_EQ(kProgram, clone->ToString()); } -TEST(AstClonerTest, ConstantArray) { +TEST(AstClonerTest, IotaArray) { constexpr std::string_view kProgram = R"(const ARRAY_SIZE = uN[32]:5; fn main() -> u32[ARRAY_SIZE] { ([u32:0, u32:1, u32:2, u32:3, u32:4]) diff --git a/xls/dslx/frontend/ast_test.cc b/xls/dslx/frontend/ast_test.cc index 7b5b21ad7a..2374a4895a 100644 --- a/xls/dslx/frontend/ast_test.cc +++ b/xls/dslx/frontend/ast_test.cc @@ -319,6 +319,9 @@ TEST(AstTest, IsConstantNumber) { EXPECT_TRUE(IsConstant(number)); } +// Tests the IsConstant predicate on an array with a single `name` reference. +// +// Since the name reference is not constant, the array is not constant. TEST(AstTest, IsConstantArrayOfNameRefs) { FileTable file_table; const Span fake_span; @@ -332,6 +335,8 @@ TEST(AstTest, IsConstantArrayOfNameRefs) { EXPECT_FALSE(IsConstant(array)); } +// Tests the IsConstant predicate on an array with a single `number` literal. +// Since the number literal is constant, the array is constant. TEST(AstTest, IsConstantArrayOfNumbers) { FileTable file_table; const Span fake_span; @@ -344,6 +349,8 @@ TEST(AstTest, IsConstantArrayOfNumbers) { EXPECT_TRUE(IsConstant(array)); } +// Tests the IsConstant predicate on an empty array. +// Since there are no members, the array is constant. TEST(AstTest, IsConstantEmptyArray) { FileTable file_table; const Span fake_span; diff --git a/xls/dslx/frontend/parser.cc b/xls/dslx/frontend/parser.cc index ad076f7ee0..c99596586e 100644 --- a/xls/dslx/frontend/parser.cc +++ b/xls/dslx/frontend/parser.cc @@ -1217,10 +1217,6 @@ absl::StatusOr Parser::ParseArray(Bindings& bindings) { } Span span(start_tok.span().start(), cbrack_pos); - if (std::all_of(exprs.begin(), exprs.end(), IsConstant)) { - return module_->Make(span, std::move(exprs), - has_trailing_ellipsis); - } return module_->Make(span, std::move(exprs), has_trailing_ellipsis); } diff --git a/xls/dslx/frontend/parser_test.cc b/xls/dslx/frontend/parser_test.cc index e0aa055048..65b55bb137 100644 --- a/xls/dslx/frontend/parser_test.cc +++ b/xls/dslx/frontend/parser_test.cc @@ -2060,7 +2060,7 @@ TEST_F(ParserTest, TupleArrayAndInt) { auto* tuple = dynamic_cast(e); EXPECT_EQ(2, tuple->members().size()); auto* array = tuple->members()[0]; - EXPECT_NE(dynamic_cast(array), nullptr); + EXPECT_NE(dynamic_cast(array), nullptr); } TEST_F(ParserTest, Cast) { RoundTripExpr("foo() as u32", {"foo"}); } @@ -2424,9 +2424,9 @@ fn f(a: MyStruct) -> u32 { })"); } -TEST_F(ParserTest, ConstantArray) { +TEST_F(ParserTest, ArrayOfConstantNumberLiterals) { Expr* e = RoundTripExpr("u32[2]:[0, 1]", {}, false, std::nullopt); - ASSERT_TRUE(dynamic_cast(e) != nullptr); + ASSERT_TRUE(dynamic_cast(e) != nullptr); } TEST_F(ParserTest, MixedArrayIsNotConstantArray) { @@ -2447,9 +2447,9 @@ const b = u32[2]:[a, a];)"); ASSERT_TRUE(dynamic_cast(array) != nullptr); } -TEST_F(ParserTest, EmptyArrayIsConstant) { +TEST_F(ParserTest, EmptyArray) { Expr* e = RoundTripExpr("u32[2]:[]", {}, false, std::nullopt); - ASSERT_TRUE(dynamic_cast(e) != nullptr); + ASSERT_TRUE(dynamic_cast(e) != nullptr); } TEST_F(ParserTest, DoubleNegation) { RoundTripExpr("!!x", {"x"}, false); } diff --git a/xls/dslx/ir_convert/extract_conversion_order.cc b/xls/dslx/ir_convert/extract_conversion_order.cc index 76059a5190..77776d72fd 100644 --- a/xls/dslx/ir_convert/extract_conversion_order.cc +++ b/xls/dslx/ir_convert/extract_conversion_order.cc @@ -236,10 +236,6 @@ class InvocationVisitor : public ExprVisitor { return absl::OkStatus(); } - absl::Status HandleConstantArray(const ConstantArray* expr) override { - return HandleArray(expr); - } - absl::Status HandleFor(const For* expr) override { XLS_RETURN_IF_ERROR(expr->init()->AcceptExpr(this)); XLS_RETURN_IF_ERROR(expr->iterable()->AcceptExpr(this)); diff --git a/xls/dslx/ir_convert/function_converter.cc b/xls/dslx/ir_convert/function_converter.cc index 1711876328..8711beb7b2 100644 --- a/xls/dslx/ir_convert/function_converter.cc +++ b/xls/dslx/ir_convert/function_converter.cc @@ -314,7 +314,6 @@ class FunctionConverterVisitor : public AstNodeVisitor { NO_TRAVERSE_DISPATCH_VISIT(Attr) NO_TRAVERSE_DISPATCH_VISIT(Array) NO_TRAVERSE_DISPATCH_VISIT(StatementBlock) - NO_TRAVERSE_DISPATCH_VISIT(ConstantArray) NO_TRAVERSE_DISPATCH_VISIT(Cast) NO_TRAVERSE_DISPATCH_VISIT(ColonRef) NO_TRAVERSE_DISPATCH_VISIT(ConstantDef) @@ -3699,14 +3698,6 @@ absl::StatusOr FunctionConverter::GetConstBits( return value.GetBitsWithStatus(); } -absl::Status FunctionConverter::HandleConstantArray(const ConstantArray* node) { - // Note: previously we would force constant evaluation here, but because all - // constexprs should be evaluated during typechecking, we shouldn't need to - // forcibly do constant evaluation at IR conversion time; therefore, we just - // build BValues and let XLS opt constant fold them. - return HandleArray(node); -} - absl::StatusOr FunctionConverter::ResolveTypeToIr( const AstNode* node) { XLS_ASSIGN_OR_RETURN(std::unique_ptr type, ResolveType(node)); diff --git a/xls/dslx/ir_convert/function_converter.h b/xls/dslx/ir_convert/function_converter.h index 8ac4baadb6..f58d2a967c 100644 --- a/xls/dslx/ir_convert/function_converter.h +++ b/xls/dslx/ir_convert/function_converter.h @@ -333,7 +333,6 @@ class FunctionConverter { absl::Status HandleStatementBlock(const StatementBlock* node); absl::Status HandleCast(const Cast* node); absl::Status HandleColonRef(const ColonRef* node); - absl::Status HandleConstantArray(const ConstantArray* node); absl::Status HandleConstantDef(const ConstantDef* node); absl::Status HandleFor(const For* node); absl::Status HandleFormatMacro(const FormatMacro* node); diff --git a/xls/dslx/type_system/deduce.cc b/xls/dslx/type_system/deduce.cc index 160750b196..5303446084 100644 --- a/xls/dslx/type_system/deduce.cc +++ b/xls/dslx/type_system/deduce.cc @@ -2030,7 +2030,6 @@ class DeduceVisitor : public AstNodeVisitor { DEDUCE_DISPATCH(Attr, DeduceAttr) DEDUCE_DISPATCH(StatementBlock, DeduceStatementBlock) DEDUCE_DISPATCH(ChannelDecl, DeduceChannelDecl) - DEDUCE_DISPATCH(ConstantArray, DeduceConstantArray) DEDUCE_DISPATCH(ColonRef, DeduceColonRef) DEDUCE_DISPATCH(Index, DeduceIndex) DEDUCE_DISPATCH(Match, DeduceMatch) diff --git a/xls/dslx/type_system/deduce_expr.cc b/xls/dslx/type_system/deduce_expr.cc index 89af50110d..b345a28645 100644 --- a/xls/dslx/type_system/deduce_expr.cc +++ b/xls/dslx/type_system/deduce_expr.cc @@ -47,9 +47,27 @@ #include "xls/ir/bits.h" namespace xls::dslx { +namespace { + +// Notes that `type` is the type of `node` in the type information and notes the +// constexpr value for the literal number in the type info. +absl::Status NoteTypeForNumber(const Number& node, const Type& type, + DeduceCtx* ctx) { + ctx->type_info()->SetItem(&node, type); + // We can't do any constexpr evaluation until parametric dims have been + // resolved. + if (type.HasParametricDims()) { + return absl::OkStatus(); + } + XLS_ASSIGN_OR_RETURN(InterpValue value, EvaluateNumber(node, type)); + ctx->type_info()->NoteConstExpr(&node, value); + return absl::OkStatus(); +} -static absl::StatusOr> DeduceEmptyArray(const Array* node, - DeduceCtx* ctx) { +} // namespace + +static absl::StatusOr> DeduceEmptyArray( + const Array* node, DeduceCtx* ctx) { // We cannot have an array that is just an ellipsis, ellipsis indicates we // should replicate the last member. if (node->has_ellipsis()) { @@ -95,17 +113,82 @@ static absl::StatusOr> DeduceEmptyArray(const Array* node, ctx->file_table()); } - return annotated; + return absl::WrapUnique(dynamic_cast(annotated.release())); } -absl::StatusOr> DeduceArray(const Array* node, - DeduceCtx* ctx) { +// Note: returns nullptr if there is no type annotation. +static absl::StatusOr> DeduceArrayTypeAnnotation( + const Array* node, DeduceCtx* ctx) { + if (node->type_annotation() == nullptr) { + return nullptr; + } + + XLS_ASSIGN_OR_RETURN(std::unique_ptr annotated, + ctx->Deduce(node->type_annotation())); + XLS_ASSIGN_OR_RETURN( + annotated, + UnwrapMetaType(std::move(annotated), node->span(), + "array type-prefix position", ctx->file_table())); + VLOG(5) << absl::StreamFormat( + "DeduceArray; inferred type annotation `%s` to be `%s`", + node->type_annotation()->ToString(), annotated->ToString()); + + // It must be an array! + auto* array_type = dynamic_cast(annotated.get()); + if (array_type == nullptr) { + return TypeInferenceErrorStatus( + node->span(), annotated.get(), + "Array was not annotated with an array type.", ctx->file_table()); + } + + if (array_type->HasParametricDims()) { + return TypeInferenceErrorStatus( + node->type_annotation()->span(), array_type, + absl::StrFormat("Annotated type for array " + "literal must be constexpr; type has dimensions that " + "cannot be resolved."), + ctx->file_table()); + } + + return absl::WrapUnique(dynamic_cast(annotated.release())); +} + +// Implemention note: there are a few cases to handle in appropriate order: +// - empty arrays we handle independently +// - if there is a type annotation that is not appropriate, we want to report +// that first instead of reporting about problems in the contents of the array +// - then we report problems with the contents of the array as normal +// +// As a user convenience, if the array type is annotated and there are bare +// numbers inside the array, we propagate the element type to the literal +// numbers. This will become more generalized with type inference 2.0. +static absl::StatusOr> DeduceArrayInternal( + const Array* node, DeduceCtx* ctx) { VLOG(5) << "DeduceArray; node: " << node->ToString(); if (node->members().empty()) { return DeduceEmptyArray(node, ctx); } + // Note: `annotated` may be nullptr. + XLS_ASSIGN_OR_RETURN(std::unique_ptr annotated, + DeduceArrayTypeAnnotation(node, ctx)); + + if (annotated != nullptr) { + // If the array type is annotated, as a user convenience, we propagate the + // element type to bare (unannotated) literal numbers contained in the + // array. + for (Expr* member : node->members()) { + if (auto* number = dynamic_cast(member); + number != nullptr && number->type_annotation() == nullptr) { + XLS_RETURN_IF_ERROR( + TryEnsureFitsInType(*number, annotated->element_type())); + XLS_RETURN_IF_ERROR( + NoteTypeForNumber(*number, annotated->element_type(), ctx)); + } + } + } + std::vector> member_types; for (Expr* member : node->members()) { XLS_ASSIGN_OR_RETURN(std::unique_ptr member_type, @@ -138,7 +221,7 @@ absl::StatusOr> DeduceArray(const Array* node, std::unique_ptr inferred = std::make_unique( member_types[0]->CloneToUnique(), member_types_dim); - if (node->type_annotation() == nullptr) { + if (annotated == nullptr) { if (node->has_ellipsis()) { const Span array_obrack_span(node->span().start(), node->span().start()); return TypeInferenceErrorStatus( @@ -154,35 +237,8 @@ absl::StatusOr> DeduceArray(const Array* node, return inferred; } - // The type annotation is present, see what it is. - XLS_ASSIGN_OR_RETURN(std::unique_ptr annotated, - ctx->Deduce(node->type_annotation())); - XLS_ASSIGN_OR_RETURN( - annotated, - UnwrapMetaType(std::move(annotated), node->span(), - "array type-prefix position", ctx->file_table())); - VLOG(5) << absl::StreamFormat( - "DeduceArray; inferred type annotation `%s` to be `%s`", - node->type_annotation()->ToString(), annotated->ToString()); - - // It must be an array! - auto* array_type = dynamic_cast(annotated.get()); - if (array_type == nullptr) { - return TypeInferenceErrorStatus( - node->span(), annotated.get(), - "Array was not annotated with an array type.", ctx->file_table()); - } - - if (array_type->HasParametricDims()) { - return TypeInferenceErrorStatus( - node->type_annotation()->span(), array_type, - absl::StrFormat("Annotated type for array " - "literal must be constexpr; type has dimensions that " - "cannot be resolved."), - ctx->file_table()); - } - - const TypeDim& annotation_size = array_type->size(); + XLS_RET_CHECK(annotated != nullptr); + const TypeDim& annotation_size = annotated->size(); // If we were presented with the wrong number of elements (vs what the // annotated type expected), flag an error. @@ -194,22 +250,22 @@ absl::StatusOr> DeduceArray(const Array* node, std::string message = absl::StrFormat( "Annotated array size %s is too small for observed array member " "count %d", - array_type->size().ToString(), member_types.size()); - return ctx->TypeMismatchError(node->span(), nullptr, *array_type, nullptr, + annotated->size().ToString(), member_types.size()); + return ctx->TypeMismatchError(node->span(), nullptr, *annotated, nullptr, *inferred, message); } } else { // No ellipsis. if (annotation_size != member_types_dim) { std::string message = absl::StrFormat( "Annotated array size %s does not match inferred array size %d.", - array_type->size().ToString(), member_types.size()); + annotated->size().ToString(), member_types.size()); if (inferred == nullptr) { // No type to compare our expectation to, as there was no member to // infer the type from. - return TypeInferenceErrorStatus(node->span(), array_type, message, + return TypeInferenceErrorStatus(node->span(), annotated.get(), message, ctx->file_table()); } - return ctx->TypeMismatchError(node->span(), nullptr, *array_type, nullptr, + return ctx->TypeMismatchError(node->span(), nullptr, *annotated, nullptr, *inferred, message); } } @@ -217,7 +273,7 @@ absl::StatusOr> DeduceArray(const Array* node, // Check the element type of the annotation is the same as the inferred type // from the element(s). XLS_ASSIGN_OR_RETURN(std::unique_ptr resolved_element_type, - ctx->Resolve(array_type->element_type())); + ctx->Resolve(annotated->element_type())); if (*resolved_element_type != *member_types[0]) { return ctx->TypeMismatchError( node->members().at(0)->span(), nullptr, *resolved_element_type, nullptr, @@ -227,69 +283,58 @@ absl::StatusOr> DeduceArray(const Array* node, } // In case of an ellipsis, the annotated type wins out. - return annotated; + return absl::WrapUnique(dynamic_cast(annotated.release())); } -absl::StatusOr> DeduceConstantArray( - const ConstantArray* node, DeduceCtx* ctx) { - if (node->type_annotation() == nullptr) { - return DeduceArray(node, ctx); +// Implementation note: we wrap up DeduceArrayInternal with a check to see the +// members are all constexpr, in which case the array itself is constexpr. +absl::StatusOr> DeduceArray(const Array* node, + DeduceCtx* ctx) { + XLS_ASSIGN_OR_RETURN(std::unique_ptr type, + DeduceArrayInternal(node, ctx)); + + VLOG(5) << "DeduceArray; node: `" << node->ToString() << "`; type: `" + << type->ToString() << "`"; + + std::vector constexpr_values; + constexpr_values.reserve(node->members().size()); + for (const Expr* member : node->members()) { + std::optional value = + ctx->type_info()->GetConstExprOption(member); + if (!value.has_value()) { + VLOG(5) << "DeduceArray; member: `" << member->ToString() + << "` is not constexpr"; + return type; + } + constexpr_values.push_back(std::move(value.value())); } - XLS_ASSIGN_OR_RETURN(std::unique_ptr type, - ctx->Deduce(node->type_annotation())); - XLS_ASSIGN_OR_RETURN( - type, UnwrapMetaType(std::move(type), node->type_annotation()->span(), - "array type-prefix position", ctx->file_table())); + VLOG(5) << "DeduceArray; constexpr_values: [" + << absl::StrJoin(constexpr_values, ", ") << "]"; - auto* array_type = dynamic_cast(type.get()); - if (array_type == nullptr) { - return TypeInferenceErrorStatus( - node->type_annotation()->span(), type.get(), - absl::StrFormat("Annotated type for array " - "literal must be an array type; got %s %s", - type->GetDebugTypeName(), - node->type_annotation()->ToString()), - ctx->file_table()); - } - - const Type& element_type = array_type->element_type(); - for (Expr* member : node->members()) { - XLS_RET_CHECK(IsConstant(member)); - if (Number* number = dynamic_cast(member); - number != nullptr && number->type_annotation() == nullptr) { - ctx->type_info()->SetItem(member, element_type); - - if (std::optional bits_like = - GetBitsLike(element_type); - bits_like.has_value()) { - XLS_RETURN_IF_ERROR( - TryEnsureFitsInType(*number, bits_like.value(), element_type)); - } else { - return TypeInferenceErrorStatus( - number->span(), &element_type, - absl::StrFormat("Annotated element type for array cannot be " - "applied to a literal number"), - ctx->file_table()); - } + // If there is an ellipsis at the end we need to fill with the last value + // until we reach the array size. + if (node->has_ellipsis()) { + XLS_ASSIGN_OR_RETURN(int64_t array_size, type->size().GetAsInt64()); + XLS_RET_CHECK(constexpr_values.size() > 0) + << "Cannot have an array with ellipsis but no given member to repeat"; + InterpValue last_value = constexpr_values.back(); + while (constexpr_values.size() < array_size) { + constexpr_values.push_back(last_value); } } - XLS_RETURN_IF_ERROR(DeduceArray(node, ctx).status()); + // We made it through all the elements observing they are constexpr -- note in + // the type information that the array is constexpr. + XLS_ASSIGN_OR_RETURN(InterpValue array_value, + InterpValue::MakeArray(std::move(constexpr_values))); + ctx->type_info()->NoteConstExpr(node, array_value); return type; } absl::StatusOr> DeduceNumber(const Number* node, DeduceCtx* ctx) { VLOG(5) << "DeduceNumber: " << node->ToString(); - auto note_constexpr_value = [&](const Type& type) -> absl::Status { - if (type.HasParametricDims()) { - return absl::OkStatus(); - } - XLS_ASSIGN_OR_RETURN(InterpValue value, EvaluateNumber(*node, type)); - ctx->type_info()->NoteConstExpr(node, value); - return absl::OkStatus(); - }; std::unique_ptr type; @@ -299,13 +344,13 @@ absl::StatusOr> DeduceNumber(const Number* node, case NumberKind::kBool: { auto type = BitsType::MakeU1(); ctx->type_info()->SetItem(node, *type); - XLS_RETURN_IF_ERROR(note_constexpr_value(*type)); + XLS_RETURN_IF_ERROR(NoteTypeForNumber(*node, *type, ctx)); return type; } case NumberKind::kCharacter: { auto type = BitsType::MakeU8(); ctx->type_info()->SetItem(node, *type); - XLS_RETURN_IF_ERROR(note_constexpr_value(*type)); + XLS_RETURN_IF_ERROR(NoteTypeForNumber(*node, *type, ctx)); return type; } default: @@ -339,8 +384,8 @@ absl::StatusOr> DeduceNumber(const Number* node, node->span(), type.get(), "Non-bits type used to define a numeric literal.", ctx->file_table()); } - ctx->type_info()->SetItem(node, *type); - XLS_RETURN_IF_ERROR(note_constexpr_value(*type)); + + XLS_RETURN_IF_ERROR(NoteTypeForNumber(*node, *type, ctx)); return type; } diff --git a/xls/dslx/type_system/deduce_expr.h b/xls/dslx/type_system/deduce_expr.h index 99f684eedc..bf338b8175 100644 --- a/xls/dslx/type_system/deduce_expr.h +++ b/xls/dslx/type_system/deduce_expr.h @@ -31,9 +31,6 @@ namespace xls::dslx { absl::StatusOr> DeduceArray(const Array* node, DeduceCtx* ctx); -absl::StatusOr> DeduceConstantArray( - const ConstantArray* node, DeduceCtx* ctx); - absl::StatusOr> DeduceNumber(const Number* node, DeduceCtx* ctx); diff --git a/xls/dslx/type_system/deduce_utils.cc b/xls/dslx/type_system/deduce_utils.cc index 859705d89c..7ddc3ca69a 100644 --- a/xls/dslx/type_system/deduce_utils.cc +++ b/xls/dslx/type_system/deduce_utils.cc @@ -205,6 +205,18 @@ absl::Status TryEnsureFitsInType(const Number& number, return absl::OkStatus(); } +absl::Status TryEnsureFitsInType(const Number& number, const Type& type) { + if (std::optional bits_like = GetBitsLike(type); + bits_like.has_value()) { + return TryEnsureFitsInType(number, bits_like.value(), type); + } + return TypeInferenceErrorStatus( + number.span(), &type, + absl::StrFormat("Non-bits type (%s) used to define a numeric literal.", + type.GetDebugTypeName()), + *number.owner()->file_table()); +} + absl::Status TryEnsureFitsInBitsType(const Number& number, const BitsType& type) { std::optional bits_like = GetBitsLike(type); diff --git a/xls/dslx/type_system/deduce_utils.h b/xls/dslx/type_system/deduce_utils.h index 7ed1b3b12f..d88222709e 100644 --- a/xls/dslx/type_system/deduce_utils.h +++ b/xls/dslx/type_system/deduce_utils.h @@ -42,11 +42,17 @@ using ColonRefSubjectT = std::variant; -// If the width is known for "type", checks that "number" fits in that type. +// Validates that "number" fits in `bits_like` if the size is known. `bits_like` +// should have been derived from `type` -- this routine uses `type` for error +// reporting. absl::Status TryEnsureFitsInType(const Number& number, const BitsLikeProperties& bits_like, const Type& type); +// Validates whether the given `type` is bits-like and, if so, delegates to +// `TryEnsureFitsInType(number, bits_like, type)`. +absl::Status TryEnsureFitsInType(const Number& number, const Type& type); + // Shorthand for the above where we know a prori the type is a `BitsType` (note // that there are types that are bits-like that are not exactly `BitsType`, see // `IsBitsLike`). diff --git a/xls/dslx/type_system/typecheck_module_test.cc b/xls/dslx/type_system/typecheck_module_test.cc index 641bc3c8e0..9efd367cf3 100644 --- a/xls/dslx/type_system/typecheck_module_test.cc +++ b/xls/dslx/type_system/typecheck_module_test.cc @@ -1910,10 +1910,11 @@ TEST(TypecheckTest, OutOfRangeNumberInConstantArray) { } TEST(TypecheckErrorTest, BadTypeForConstantArrayOfNumbers) { - EXPECT_THAT(Typecheck("const A = u8[3][4]:[1, 2, 3, 4];"), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("Annotated element type for array cannot be " - "applied to a literal number"))); + EXPECT_THAT( + Typecheck("const A = u8[3][4]:[1, 2, 3, 4];"), + StatusIs( + absl::StatusCode::kInvalidArgument, + HasSubstr("Non-bits type (array) used to define a numeric literal"))); } TEST(TypecheckErrorTest, ConstantArrayEmptyMembersWrongCountVsDecl) { @@ -2263,14 +2264,14 @@ fn main() -> () { } TEST(TypecheckTest, BadArrayLiteralType) { - EXPECT_THAT(Typecheck(R"( + EXPECT_THAT( + Typecheck(R"( fn main() -> s32[2] { s32:[1, 2] } )"), - StatusIs(absl::StatusCode::kInvalidArgument, - HasSubstr("Annotated type for array literal must be an " - "array type; got sbits s32"))); + StatusIs(absl::StatusCode::kInvalidArgument, + HasSubstr("Array was not annotated with an array type"))); } TEST(TypecheckTest, CharLiteralArray) { diff --git a/xls/tools/proto_to_dslx.cc b/xls/tools/proto_to_dslx.cc index 29a509ccfd..12b7d0f7c3 100644 --- a/xls/tools/proto_to_dslx.cc +++ b/xls/tools/proto_to_dslx.cc @@ -342,7 +342,7 @@ absl::StatusOr MakeZeroValuedElement( const dslx::FileTable& file_table = *module->file_table(); XLS_ASSIGN_OR_RETURN(uint64_t real_size, array_size->GetAsUint64(file_table)); - return module->Make( + return module->Make( span, std::vector(real_size, member), /*has_ellipsis=*/false); } @@ -708,8 +708,7 @@ absl::Status EmitArray( } } - auto* array = - module->Make(span, *array_elements, has_ellipsis); + auto* array = module->Make(span, *array_elements, has_ellipsis); elements->push_back(std::make_pair(std::string(field_name), array)); auto* u32_type = module->Make( From ce57dad5cef97b5744b8df2e90ee4da34c84cb2c Mon Sep 17 00:00:00 2001 From: Chris Leary Date: Fri, 29 Nov 2024 11:40:35 -0800 Subject: [PATCH 2/2] Make a comment and add a test for one place where IsConstant is still used. --- xls/dslx/frontend/parser.cc | 7 +++++++ xls/dslx/type_system/typecheck_module_test.cc | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/xls/dslx/frontend/parser.cc b/xls/dslx/frontend/parser.cc index c99596586e..9ed48a97e6 100644 --- a/xls/dslx/frontend/parser.cc +++ b/xls/dslx/frontend/parser.cc @@ -1268,11 +1268,18 @@ absl::StatusOr Parser::ParseCast(Bindings& bindings, return term; } + // This handles the case where we're trying to make a tuple constant that has + // an explicitly annotated type; e.g. + // + // ```dslx + // const MY_TUPLE = (u32, u64):(u32:32, u64:64); + // ``` if (auto* tuple = dynamic_cast(term); tuple != nullptr && std::all_of(tuple->members().begin(), tuple->members().end(), IsConstant)) { return term; } + return ParseErrorStatus( type->span(), "Old-style cast only permitted for constant arrays/tuples " diff --git a/xls/dslx/type_system/typecheck_module_test.cc b/xls/dslx/type_system/typecheck_module_test.cc index 9efd367cf3..8cb41da5c6 100644 --- a/xls/dslx/type_system/typecheck_module_test.cc +++ b/xls/dslx/type_system/typecheck_module_test.cc @@ -282,6 +282,13 @@ fn f() -> u32 { p() } XLS_EXPECT_OK(Typecheck(kProgram)); } +TEST(TypecheckTest, TupleWithExplicitlyAnnotatedType) { + constexpr std::string_view kProgram = R"( +const MY_TUPLE = (u32, u64):(u32:32, u64:64); +)"; + XLS_EXPECT_OK(Typecheck(kProgram)); +} + TEST(TypecheckTest, ProcWithImplEmpty) { constexpr std::string_view kProgram = R"( proc Foo {}