Skip to content
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

Add error message for obscure case with parameters. #214

Merged
merged 2 commits into from
Dec 18, 2024
Merged

Add error message for obscure case with parameters. #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cd279b
Add error message for obscure case with parameters.
reventlov Dec 13, 2024
c53b59b
Reformat with Black.
reventlov Dec 13, 2024
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
71 changes: 55 additions & 16 deletions compiler/front_end/constraints.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,49 @@ def _check_type_requirements_for_field(
)


def _check_early_type_requirements_for_parameter_type(
runtime_parameter, ir, source_file_name, errors
):
"""Checks that the type of a parameter is valid."""
physical_type = runtime_parameter.physical_type_alias
logical_type = runtime_parameter.type
size = ir_util.constant_value(physical_type.size_in_bits)
if logical_type.which_type == "integer":
# This seems a little weird: for `UInt`, `Int`, etc., the explicit size
# is required, but for enums it is banned. This is because enums have
# a "natural" size (the size specified in the `enum` definition, or 64
# bits by default) in expressions, so the physical size would just be
# ignored. Integer types do not have "natural" sizes, so the width is
# required.
if not physical_type.HasField("size_in_bits"):
errors.extend(
[
[
error.error(
source_file_name,
physical_type.source_location,
f"Parameters with integer type must have explicit size (e.g., `{physical_type.atomic_type.reference.source_name[-1].text}:32`).",
)
]
]
)
elif logical_type.which_type == "enumeration":
if physical_type.HasField("size_in_bits"):
errors.extend(
[
[
error.error(
source_file_name,
physical_type.size_in_bits.source_location,
"Parameters with enum type may not have explicit size.",
)
]
]
)
else:
assert False, "Non-integer/enum parameters should have been caught earlier."


def _check_type_requirements_for_parameter_type(
runtime_parameter, ir, source_file_name, errors
):
Expand Down Expand Up @@ -346,22 +389,7 @@ def _check_type_requirements_for_parameter_type(
)
)
elif logical_type.which_type == "enumeration":
if physical_type.HasField("size_in_bits"):
# This seems a little weird: for `UInt`, `Int`, etc., the explicit size is
# required, but for enums it is banned. This is because enums have a
# "native" 64-bit size in expressions, so the physical size is just
# ignored.
errors.extend(
[
[
error.error(
source_file_name,
physical_type.size_in_bits.source_location,
"Parameters with enum type may not have explicit size.",
)
]
]
)
pass
else:
assert False, "Non-integer/enum parameters should have been caught earlier."

Expand Down Expand Up @@ -686,6 +714,17 @@ def _attribute_in_attribute_action(a):
return {"in_attribute": a}


def check_early_constraints(ir):
errors = []
traverse_ir.fast_traverse_ir_top_down(
ir,
[ir_data.RuntimeParameter],
_check_early_type_requirements_for_parameter_type,
parameters={"errors": errors},
)
return errors


def check_constraints(ir):
"""Checks miscellaneous validity constraints in ir.

Expand Down
48 changes: 39 additions & 9 deletions compiler/front_end/constraints_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@
from compiler.util import test_util


def _make_ir_from_emb(emb_text, name="m.emb"):
def _make_ir_from_emb(emb_text, name="m.emb", stop_before_step="check_constraints"):
ir, unused_debug_info, errors = glue.parse_emboss_file(
name,
test_util.dict_file_reader({name: emb_text}),
stop_before_step="check_constraints",
stop_before_step=stop_before_step,
)
assert not errors, repr(errors)
return ir
Expand Down Expand Up @@ -1252,8 +1252,12 @@ def test_checks_constancy_of_constant_references(self):
error.filter_errors(constraints.check_constraints(ir)),
)

def test_checks_for_explicit_size_on_parameters(self):
ir = _make_ir_from_emb("struct Foo(y: UInt):\n" " 0 [+1] UInt x\n")
def test_checks_for_explicit_integer_size_on_parameters(self):
ir = _make_ir_from_emb(
"struct Foo(y: UInt):\n" #
" 0 [+1] UInt x\n",
stop_before_step="check_early_constraints",
)
error_parameter = ir.module[0].type[0].runtime_parameter[0]
error_location = error_parameter.physical_type_alias.source_location
self.assertEqual(
Expand All @@ -1262,12 +1266,34 @@ def test_checks_for_explicit_size_on_parameters(self):
error.error(
"m.emb",
error_location,
"Integer range of parameter must not be unbounded; it "
"must fit in a 64-bit signed or unsigned integer.",
"Parameters with integer type must have explicit size (e.g., `UInt:32`).",
)
]
],
error.filter_errors(constraints.check_constraints(ir)),
error.filter_errors(constraints.check_early_constraints(ir)),
)

def test_checks_for_explicit_integer_size_on_parameters_and_uses_type_in_error(
self,
):
ir = _make_ir_from_emb(
"struct Foo(y: Int):\n" #
" 0 [+1] UInt x\n",
stop_before_step="check_early_constraints",
)
error_parameter = ir.module[0].type[0].runtime_parameter[0]
error_location = error_parameter.physical_type_alias.source_location
self.assertEqual(
[
[
error.error(
"m.emb",
error_location,
"Parameters with integer type must have explicit size (e.g., `Int:32`).",
)
]
],
error.filter_errors(constraints.check_early_constraints(ir)),
)

def test_checks_for_correct_explicit_size_on_parameters(self):
Expand All @@ -1292,7 +1318,11 @@ def test_checks_for_correct_explicit_size_on_parameters(self):

def test_checks_for_explicit_enum_size_on_parameters(self):
ir = _make_ir_from_emb(
"struct Foo(y: Bar:8):\n" " 0 [+1] UInt x\n" "enum Bar:\n" " QUX = 1\n"
"struct Foo(y: Bar:8):\n" #
" 0 [+1] UInt x\n"
"enum Bar:\n"
" QUX = 1\n",
stop_before_step="check_early_constraints",
)
error_parameter = ir.module[0].type[0].runtime_parameter[0]
error_size = error_parameter.physical_type_alias.size_in_bits
Expand All @@ -1307,7 +1337,7 @@ def test_checks_for_explicit_enum_size_on_parameters(self):
)
]
],
error.filter_errors(constraints.check_constraints(ir)),
error.filter_errors(constraints.check_early_constraints(ir)),
)


Expand Down
5 changes: 4 additions & 1 deletion compiler/front_end/expression_bounds.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,18 +144,21 @@ def _compute_constraints_of_field_reference(expression, ir):
expression.type.integer.modulus = "1"
expression.type.integer.modular_value = "0"
type_definition = ir_util.find_parent_object(field_path, ir)
type_size = None
if isinstance(field, ir_data.Field):
referrent_type = field.type
else:
referrent_type = field.physical_type_alias
if referrent_type.HasField("size_in_bits"):
type_size = ir_util.constant_value(referrent_type.size_in_bits)
else:
elif isinstance(field, ir_data.Field):
field_size = ir_util.constant_value(field.location.size)
if field_size is None:
type_size = None
else:
type_size = field_size * type_definition.addressable_unit
else:
type_size = None
assert referrent_type.HasField("atomic_type"), field
assert not referrent_type.atomic_type.reference.canonical_name.module_file
_set_integer_constraints_from_physical_type(
Expand Down
1 change: 1 addition & 0 deletions compiler/front_end/glue.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,7 @@ def process_ir(ir, stop_before_step):
symbol_resolver.resolve_field_references,
type_check.annotate_types,
type_check.check_types,
constraints.check_early_constraints,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity does this additional pass add much overhead to the end-to-end codegen time?

Copy link
Collaborator Author

@reventlov reventlov Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

> python3 -m cProfile ./embossc --output-path ~/scratch ~/scratch/large.emb

         25766374 function calls (23764959 primitive calls) in 17.613 seconds

   Ordered by: cumulative time

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
    181/1    0.014    0.000   18.280   18.280 {built-in method builtins.exec}
[...]
        1    0.000    0.000    0.003    0.003 constraints.py:717(check_early_constraints)
[...]
        8    0.000    0.000    0.002    0.000 constraints.py:365(_check_type_requirements_for_parameter_type)
[...]

Looks like <0.02% of the overall compilation time. large.emb does have a few parameters, but is not parameter-heavy; I imagine that an .emb with a lot of parameters would pay a correspondingly higher compile time cost.

expression_bounds.compute_constants,
attribute_checker.normalize_and_verify,
constraints.check_constraints,
Expand Down
Loading