-
Notifications
You must be signed in to change notification settings - Fork 50
explicit_object_extensions provisional -- SIMICS-23313
#399
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1147,6 +1147,7 @@ class ENOVERRIDEPARAM(DMLError): | |
| To declare and define a new parameter not already declared, use the `:=` or | ||
| `:default` syntax. | ||
| """ | ||
| version = "1.4" | ||
| fmt = ("parameter '%s' not declared previously." | ||
| " To declare and define a new parameter, use the ':%s' syntax.") | ||
|
|
||
|
|
@@ -1167,6 +1168,7 @@ class EOVERRIDEPARAM(DMLError): | |
| to override existing parameter declarations (not even those lacking a | ||
| definition of the parameter.) | ||
| """ | ||
| version = "1.4" | ||
| fmt = ("the parameter '%s' has already been declared " | ||
| + "(':%s' syntax may not be used for parameter overrides)") | ||
| def __init__(self, site, other_site, name, token): | ||
|
|
@@ -1176,6 +1178,45 @@ def log(self): | |
| DMLError.log(self) | ||
| self.print_site_message(self.other_site, "existing declaration") | ||
|
|
||
|
|
||
| class EEXTENSION(DMLError): | ||
| """When the [`explicit_object_extensions` provisional | ||
| feature](provisional-auto.html#explicit_object_extensions) is enabled, | ||
| any object definition made via `in` syntax is considered an extension such | ||
| that there must be some other non-extension declaration of the object, or | ||
| DMLC will reject the extension. | ||
| To declare and define a new object not already declared, omit the `in` | ||
| syntax. | ||
| """ | ||
| version = "1.4" | ||
| fmt = ("object '%s' not declared elsewhere." | ||
| " To declare and define a new object, omit 'in'.") | ||
|
|
||
| class EMULTIOBJDECL(DMLError): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I struggled a bit with naming of these error tags, but ultimately it doesn't matter much. |
||
| """When the [`explicit_object_extensions` provisional | ||
| feature](provisional-auto.html#explicit_object_extensions) is enabled, | ||
| any object declaration not made using `in` syntax is considered a | ||
| declaration of a novel object — because of that, DMLC will reject | ||
| it if there already is another non-`in` declaration across files enabling | ||
| `explicit_object_extensions`. | ||
| """ | ||
| version = "1.4" | ||
| fmt = ("object '%s' already declared." | ||
| " To extend upon the definition of an object, use 'in %s'") | ||
| def __init__(self, site, other_site, objtype, name): | ||
| super().__init__(site, name, f'{objtype} {name} ...') | ||
| self.other_site = other_site | ||
|
|
||
| def log(self): | ||
| from . import provisional | ||
| DMLError.log(self) | ||
| self.print_site_message(self.other_site, "existing declaration") | ||
| prov_site = self.site.provisional_enabled( | ||
| provisional.explicit_object_extensions) | ||
| self.print_site_message( | ||
| prov_site, | ||
| "enabled by the explicit_object_extensions provisional feature") | ||
|
|
||
| class EVARPARAM(DMLError): | ||
| """ | ||
| The value assigned to the parameter is not a well-defined constant. | ||
|
|
@@ -1285,6 +1326,7 @@ class ENOVERRIDEMETH(DMLError): | |
| To declare and define a new method not already declared, use the `:{ ... }` | ||
| or `:default { ... }` syntax. | ||
| """ | ||
| version = "1.4" | ||
| fmt = ("method '%s' not declared previously." | ||
| " To declare and define a new method, use the ':%s{...}' syntax.") | ||
|
|
||
|
|
@@ -1304,6 +1346,7 @@ class EOVERRIDEMETH(DMLError): | |
| can't be used to override existing parameter declarations (not even those | ||
| lacking a definition of the parameter.) | ||
| """ | ||
| version = "1.4" | ||
| fmt = ("the method '%s' has already been declared " | ||
| + "(':%s{ ... }' syntax may not be used for method overrides)") | ||
| def __init__(self, site, other_site, name, token): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -504,9 +504,9 @@ def wrap_sites(spec, issite, tname): | |
| else: | ||
| raise ICE(issite, 'unknown node type %r %r' % (asttype, stmt)) | ||
| composite_wrapped = [ | ||
| (objtype, name, arrayinfo, | ||
| (objtype, name, arrayinfo, is_extension, | ||
| ObjectSpec(*wrap_sites(spec, issite, tname))) | ||
| for (objtype, name, arrayinfo, spec) in composite] | ||
| for (objtype, name, arrayinfo, is_extension, spec) in composite] | ||
| blocks.append((preconds, shallow_wrapped, composite_wrapped)) | ||
|
|
||
| return (TemplateSite(spec.site, issite, tname), spec.rank, | ||
|
|
@@ -813,8 +813,8 @@ def report_poverride(sup, inf, obj_specs): | |
| report(POVERRIDE_IMPORT(sup_obj.site, inf.desc.text)) | ||
|
|
||
| def merge_subobj_defs(def1, def2, parent): | ||
| (objtype, name, arrayinfo, obj_specs1) = def1 | ||
| (objtype2, name2, arrayinfo2, obj_specs2) = def2 | ||
| (objtype, name, arrayinfo, extension_status1, obj_specs1) = def1 | ||
| (objtype2, name2, arrayinfo2, extension_status2, obj_specs2) = def2 | ||
| assert name == name2 | ||
|
|
||
| site1 = obj_specs1[0].site | ||
|
|
@@ -824,6 +824,26 @@ def merge_subobj_defs(def1, def2, parent): | |
| report(ENAMECOLL(site1, site2, name)) | ||
| return def1 | ||
|
|
||
| # extension status: | ||
| # None -> dual extension and decl | ||
| # True -> extension | ||
| # False -> explicit decl | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ternary rep is pretty damn ugly. @mandolaerik does this deserve the use of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current rep is fine, but I also wouldn't object to enum. The question it poses to me is rather whether it makes sense to handle subobj merge in this incremental pairwise fashion -- I think this logic would be much easier to express if we just merged all object defs at once. IIRC the pairwise thing is there for a historical reason: before dml 1.4, templates would consist of pre-merged subobj defs, so a template used many times did not have to merge defs again. It would probably make sense to refactor all of merge_subobj_defs, but no need to do within this PR. |
||
| if extension_status1 is extension_status2 is False: | ||
| # Blame the one with higher rank if possible. Otherwise, blame the | ||
| # new def. | ||
| (decl_site_1, decl_site_2) = ( | ||
| (site1, site2) if ( | ||
| obj_specs2[0].rank in obj_specs1[0].rank.inferior) | ||
| else (site2, site1)) | ||
| report(EMULTIOBJDECL(decl_site_1, decl_site_2, objtype, name)) | ||
| # One is explicit decl: merged is explicit decl | ||
| elif extension_status1 is False or extension_status2 is False: | ||
| extension_status1 = False | ||
| # If either is dual, make dual | ||
| elif extension_status1 is None or extension_status2 is None: | ||
| extension_status1 = None | ||
| # Otherwise, both are explicit extensions. Keep explicit extension. | ||
|
|
||
| if len(arrayinfo) != len(arrayinfo2): | ||
| raise EAINCOMP(site1, site2, name, | ||
| "mixing declarations with different number " | ||
|
|
@@ -850,7 +870,8 @@ def merge_subobj_defs(def1, def2, parent): | |
| merged_arrayinfo.append((idxvar1, len1)) | ||
|
|
||
|
|
||
| return (objtype, name, merged_arrayinfo, obj_specs1 + obj_specs2) | ||
| return (objtype, name, merged_arrayinfo, extension_status1, | ||
| obj_specs1 + obj_specs2) | ||
|
|
||
| def method_is_std(node, methname): | ||
| """ | ||
|
|
@@ -1786,13 +1807,14 @@ def mkobj2(obj, obj_specs, params, each_stmts): | |
|
|
||
| for (stmts, obj_spec) in composite_subobjs: | ||
| for s in stmts: | ||
| (objtype, ident, arrayinfo, subobj_spec) = s | ||
| (objtype, ident, arrayinfo, is_extension, subobj_spec) = s | ||
|
|
||
| if ident is None: | ||
| assert (dml.globals.dml_version == (1, 2) | ||
| and objtype in {'bank', 'field'}) | ||
|
|
||
| subobj_def = (objtype, ident, arrayinfo, [subobj_spec]) | ||
| subobj_def = (objtype, ident, arrayinfo, is_extension, | ||
| [subobj_spec]) | ||
| if ident in subobj_defs: | ||
| subobj_defs[ident] = merge_subobj_defs(subobj_defs[ident], | ||
| subobj_def, obj) | ||
|
|
@@ -1802,7 +1824,10 @@ def mkobj2(obj, obj_specs, params, each_stmts): | |
| symbols[ident] = subobj_spec.site | ||
| subobj_defs[ident] = subobj_def | ||
|
|
||
| for (_, _, arrayinfo, specs) in subobj_defs.values(): | ||
| for (_, ident, arrayinfo, extension_status, specs) in subobj_defs.values(): | ||
| if extension_status is True: | ||
| for spec in specs: | ||
| report(EEXTENSION(spec.site, ident)) | ||
| for (i, (idx, dimsize_ast)) in enumerate(arrayinfo): | ||
| if dimsize_ast is None: | ||
| idxref = (f" (with index variable '{idx.args[0]}')" | ||
|
|
@@ -1904,7 +1929,7 @@ def mkobj2(obj, obj_specs, params, each_stmts): | |
| # the whole register. | ||
| if obj.objtype == 'register' and not any( | ||
| objtype == 'field' | ||
| for (objtype, _, _, _) in list(subobj_defs.values())): | ||
| for (objtype, _, _, _, _) in list(subobj_defs.values())): | ||
| # The implicit field instantiates the built-in field | ||
| # template and does nothing else. | ||
| subobjs.append(mkobj( | ||
|
|
@@ -1946,7 +1971,7 @@ def mkobj2(obj, obj_specs, params, each_stmts): | |
| subobj_name_defs = {} | ||
|
|
||
| for name in sorted(subobj_defs, key=lambda name: name or ''): | ||
| (objtype, ident, arrayinfo, subobj_specs) = subobj_defs[name] | ||
| (objtype, ident, arrayinfo, _, subobj_specs) = subobj_defs[name] | ||
| if (not obj.accepts_child_type(objtype) | ||
| # HACK: disallow non-toplevel banks in DML 1.2, see SIMICS-19009 | ||
| or (dml.globals.dml_version == (1, 2) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really want to allow
in register x size 4 @ 4711 { }? all the special syntax is a clear declaration marker to me.I suggest
in KIND NAME [INDICES] { ... }as the only allowed form, analogous to the grammar forin each xyz { ... }BTW: should we forbid the
[_ < ...]index forms outside extensions when the provisional is active?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reference this in the JIRA issue. I intentionally made this feature as consistent as possible, and the questions of what we should or should not allow a separate issue -- because the answers are not obvious, (in particular, although I previously agreed with forbidding
[_ < ...]as a decl, upon reevaluation it's less obvious to me that such a decl would be nonsensical.)In other words: i don't care when it comes to this PR. To answer these questions, we need deeper discussion and analysis, and this PR has no need for that since this is an unstable provisional. Besides, the consistent design is a virtue in its own right, and answers about it vs. any ad-hoc restrictions we make could perhaps be enlightened by Gustav's use of it.