Skip to content

Remove WEXPERIMENTAL for "if with unsupported statements" #332

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions RELEASENOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -204,3 +204,11 @@
(fixes SIMICS-22848).
- `release 6 6349`
- `release 7 7054`
- `note 6` DMLC no longer emits warnings saying
Copy link
Contributor

Choose a reason for hiding this comment

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

will no longer emit the subset of WEXPERIMENTAL warnings saying...

maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I chose not to because I didn't want to bore the reader with details, but it does make sense to mention WEXPERIMENTAL because of how --no-warn=WEXPERIMENTAL is sprinkled in makefiles.

Choose a reason for hiding this comment

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

Yeah that's mostly why I figured it was a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Toplevel if logic is 1.4 exclusive, and thus so should this releasenote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree. You are right that they are exclusive for 1.4 files, but their only use is in the context of 1.2 devices: The only allowed identifier is dml_1_2, so the construct is only relevant for common code intended for use in 1.2.

Copy link
Contributor

@lwaern-intel lwaern-intel Nov 21, 2024

Choose a reason for hiding this comment

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

so the construct is only relevant for common code intended for use in 1.2.

Only for the time being. I think we've both agreed upon extending the conditional to also allow the use of the compat params?

Nevertheless, I guess this is a matter of perspective of where a releasenote belongs. I was thinking "1.4, because this feature is only available in 1.4 files", while you're arguing the use of it is only significant due to how it impact 1.2 devices. Because of that, I'm fine with it being in RELEASENOTES.dml; but I would note that your argument sounds like it'd belong in RELEASENOTES-1.2.md, which I would heavily disagree with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The releasenote split between 1.2/1.4/both is inherently fuzzy and somewhat questionable. What matters the most is that the end-user sees a note attached to the right package version, this happens no matter what releasenote file we pick; the choice of file is not so important.

I guess the advantage of the split is that many syntax additions are 1.4 specific, and putting it in -1.4 eliminates the need to mention this in text.

I think it would make sense at least to merge RELEASENOTES and RELEASENOTES-1.2. A total 7 notes were 1.2 specific over the past 3 years.

`top-level 'if' body with unsupported statements`. These warnings were
often triggered in common code, causing excessive polution in build logs.
The same rules as before apply to `#if` statements: Statements such as
`param` and `template` are forbidden inside `#if`, but a special exception
allows forbidden statements to appear specifically inside an `#if (dml_1_2)`
block. The warning message was meant to highlight this irregularity, but
caused more harm than good; error messages surrounding the special case have been improved instead.
30 changes: 26 additions & 4 deletions doc/1.4/language.md
Original file line number Diff line number Diff line change
Expand Up @@ -2318,8 +2318,8 @@ The *object declarations* are any number of declarations of objects, session
variables, saved variables, methods, or other `#if` statements, but not
parameters, `is` statements, or `in each` statements . When the conditional is
`true` (or if it's the else branch of a false conditional), the object
declarations are treated as if they had appeared without any surrounding *#if*.
So the two following declarations are equivalent:
declarations are treated as if they had appeared without any surrounding `#if`.
Thus, the two following snippets are equivalent:

```
#if (true) {
Expand All @@ -2329,12 +2329,34 @@ So the two following declarations are equivalent:
}
```

is equivalent to

```
register R size 4;
```

As a special exception, an `#if` statement that appears on top level is allowed
to contain any type of statement, as long as the condition doesn't reference
any identifiers other than `dml_1_2`, `true` and `false`. This is often useful
while migrating the devices of a system from DML 1.2 to DML 1.4,
as it allows conditional definitions of templates in common code used from both DML 1.2 and DML 1.4. For example, let's say an existing template `reset_to_seven`
Copy link
Contributor

Choose a reason for hiding this comment

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

line surpasses 79 char width limit

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the same style rule applies for .md; IIRC @aconradi suggested "new line between sentences" as an alternative convention to make diffs more manageable.

For markdown embedded in source comments, the host language's conventions take precedence of course.

Copy link

@KingoftheHomeless KingoftheHomeless Oct 26, 2024

Choose a reason for hiding this comment

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

Whichever criteria you choose, you fail it. If nothing else, "For example," should be on a new line.

I still vote for breaking on spaces except when ` is involved, because apparently the GH wiki inteprets line breaks within tick literally (which we need to clean up...)

The reason I vote for it is consistency with the rest of the document, as well as a slight preference that the boundary is consistent throughout the file (as opposed to natural linebreaks, since sentences will be of differing lengths.)

is used in DML 1.2 code to set the reset value of a field to 7 in DML 1.2.
The parameters that control reset values have changed from DML 1.2 to DML 1.4,
and one way to handle this is to provide separate template definitions
depending on whether the device uses DML 1.2 or DML 1.4:
```
#if (dml_1_2) {
template seven is field {
param hard_reset_value = 7;
param soft_reset_value = 7;
}
} #else {
template seven is field {
param init_val = 7;
}
}
```
Later, when all related devices have been ported to DML 1.4,
the `dml_1_2` clause can be removed.

## In Each Declarations

In Each declarations are a convenient mechanism to apply a
Expand Down
10 changes: 5 additions & 5 deletions py/dml/dmlparse.py
Original file line number Diff line number Diff line change
Expand Up @@ -270,12 +270,12 @@ def toplevel_param(t):
def toplevel_if(t):
'''toplevel_if : hashif LPAREN expression RPAREN \
LBRACE device_statements RBRACE toplevel_else'''
if all(stmt.kind in allowed_in_hashif for stmt in t[6] + t[8]):
t[0] = ast.hashif(site(t), t[3], t[6], t[8])
bad_stmts = [stmt for stmt in t[6] + t[8]
if stmt.kind not in allowed_in_hashif]
if bad_stmts:
t[0] = ast.toplevel_if(site(t), t[3], t[6], t[8], bad_stmts)
else:
report(WEXPERIMENTAL(site(t), ("top-level 'if' body with unsupported "
+ "statements")))
t[0] = ast.toplevel_if(site(t), t[3], t[6], t[8])
t[0] = ast.hashif(site(t), t[3], t[6], t[8])

@prod_dml14
def toplevel_else_no(t):
Expand Down
12 changes: 12 additions & 0 deletions py/dml/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,18 @@ class ECONDINEACH(DMLError):
version = "1.4"
fmt = "conditional 'in each' is not allowed"

class EBADCONDSTMT(DMLError):
"""
`#if` statements in object scope are only allowed to contain
certain kinds of declarations: objects, `method`, `session`,
`saved`, `#if`, or `error`.

A special exception is that a `#if` on top scope may contain any
kind of statement as long as the `#if` condition doesn't reference
any identifiers other than `dml_1_2`, `true`, and `false`.
"""
fmt = "object of type %s not allowed inside `#if`"
Copy link
Contributor

Choose a reason for hiding this comment

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

%s declarations are not allowed inside #if

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe %s statements are not ...? Note that this is a best-effort error message, will e.g. say "extern_typedef statements are ..." which is not optimal but good enough. The important ones are param, import and template, and we get those right at least.

Copy link

@KingoftheHomeless KingoftheHomeless Oct 26, 2024

Choose a reason for hiding this comment

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

How come you prefer statements instead of declarations? Sure, we have a concept of "object statements", but as you point out, many of the forbidden things doesn't fit inside that concept; and we have "declaration" as a collective name for object statements and global declarations (which sometimes is not ideal; for example in our documentation we often say "param declarations" as a collective name for all param object statements, which I dislike, but that's what we do. If we say "param declarations not allowed inside #if" I don't think people would get confused.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, this error message should be tagged version = "1.4". Toplevel if logic can only happen in 1.4 code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized. The emission of this error message disagrees with non-toplevel ifs. They crop up from invocations of validate_if_body() from dmlparse.py; and those use ECONDP, ECONDT and ECONDINEACH. We'll have to address that; replace them all with EBADCONDSTMT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a contradiction here. The messages for non-top-level ifs do not need to mention that top-level ifs have a special case. They are also more specific than top-level ifs, and I don't see a problem with that. I would probably not oppose unifying the messages, but that can be done in a separate PR.

Copy link
Contributor

@lwaern-intel lwaern-intel Nov 21, 2024

Choose a reason for hiding this comment

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

The messages for non-top-level ifs do not need to mention that top-level ifs have a special case.

Well, no, I'm not arguing that (we've settled that a time ago.) I'm confused why you're bringing that up; what are you addressing with that statement? Or are you talking about documentation -- "the documentation for error messages of non-top-level ifs do not need to mention that top-level ifs have a special case, thus it's a good thing they're separate error classes?" I don't buy that argument at all.

They are also more specific than top-level ifs, and I don't see a problem with that.

Neither do I. What I have a problem with is that your PR introduces an inconsistency about what error classes are used for reporting the same kind of error (from the user's perspective.) Because of that, I don't buy the argument that unification can be done in a separate PR.
Thus I'm arguing either you replace ECONDP, ECONDT, and ECONDINEACH all with EBADCONDSTMT, or you replace EBADCONDSTMT with uses of ECONDP, ECONDT, and ECONDINEACH. Either way, you can implement it by having ast builder pass what forbidden declarations are in use, and then you can either report ECONDP/ECONDT/ECONDINEACH for each of them; or you can pass the sites and declaration kinds to EBADCONDSTMT to each be printed via print_site_message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I have a problem with is that your PR introduces an inconsistency

It's not an inconsistency toward the end user. The same kind of mistake will always give an error, and in some circumstances it will use a somewhat different wording (essentially because there's more error cases to cover). The end result is that the user will read the message and adjust their code, and then they will not see the message again. What's the problem? The error tags are mainly related to our internal testing.

Hm, what we could do is to pick a more directed message, like "not allowed inside if, except dml_1_2" if the violating statement is one that only can appear on top level, like a typedef or template. And use a plain ECONDP etc for the other cases. Not sure if that makes anyone happier, though.


# TODO: Consider re-wording the semantics of this error, allocate_type is only
# relevant in 1.4 when imported from 1.2, and as per SIMICS-9393 this
# error might not even be necessary
Expand Down
18 changes: 11 additions & 7 deletions py/dml/toplevel.py
Original file line number Diff line number Diff line change
Expand Up @@ -148,25 +148,29 @@ def scan_statements(filename, site, stmts):
[text] = s.args
footers.append(ctree.mkCText(s.site, text))
elif s.kind == 'toplevel_if':
[cond, tbranch, fbranch] = s.args
[cond, tbranch, fbranch, bad_stmts] = s.args
scope = symtab.Symtab()
bsite = SimpleSite('<builtin>',
dml_version=dml.globals.dml_version)
# HACK Add constants to scope typically defined by dml-builtins,
# which is not accessible here
def add_constant(name, expr):
constants = {
'dml_1_2': ctree.BoolConstant(
bsite, dml.globals.dml_version == (1, 2)),
'true': ctree.BoolConstant(bsite, True),
'false': ctree.BoolConstant(bsite, False),
}
for (name, expr) in constants.items():
scope.add(ctree.ExpressionSymbol(name, expr, bsite))

add_constant(
'dml_1_2',
ctree.BoolConstant(bsite, dml.globals.dml_version == (1, 2)))
add_constant('true', ctree.BoolConstant(bsite, True))
add_constant('false', ctree.BoolConstant(bsite, False))
try:
expr = ctree.as_bool(codegen.codegen_expression(
cond, None, scope))
if not expr.constant:
raise ENCONST(expr.site, expr)
except EIDENT:
for stmt in bad_stmts:
report(EBADCONDSTMT(stmt.site, stmt.kind))
except DMLError as e:
report(e)
else:
Expand Down
1 change: 0 additions & 1 deletion test/1.2/misc/T_import_dml14.dml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ constant x = 0;

parameter is_dml_12 = true;

/// WARNING WEXPERIMENTAL dml14.dml
/// SCAN-FOR-TAGS dml14.dml
import "dml14.dml";

Expand Down
3 changes: 0 additions & 3 deletions test/1.2/misc/dml14.dml
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,6 @@ bank testbank is (miss_pattern_bank, function_mapped_bank) {

bank overridden_bank is (unified_hard_reset, unified_soft_reset);

/// WARNING WEXPERIMENTAL
#if (dml_1_2) {
import "io-memory.dml";
} #else {
Expand Down Expand Up @@ -475,10 +474,8 @@ attribute woa is (write_only_attr) {

param global_sym = 14;

/// WARNING WEXPERIMENTAL
#if (!dml_1_2) {
param cond = false;
/// WARNING WEXPERIMENTAL
} #else #if (!dml_1_2) {
error;
} #else {
Expand Down
28 changes: 28 additions & 0 deletions test/1.4/errors/T_EBADCONDSTMT.dml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
© 2024 Intel Corporation
SPDX-License-Identifier: MPL-2.0
*/
dml 1.4;

device test;

constant x = 1;

#if (x == 1) {
/// ERROR EBADCONDSTMT
param p = 3;
/// ERROR EBADCONDSTMT
typedef int i_t;
} #else {
/// ERROR EBADCONDSTMT
template t {
}

/// ERROR EBADCONDSTMT
extern typedef int i_t;
}

#if (true) {
// no error
param p = 3;
}
1 change: 0 additions & 1 deletion test/1.4/errors/T_ECONDINEACH.dml
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ port p {
}
}

/// WARNING WEXPERIMENTAL
#if (false && true && dml_1_2) {
// Experimental support for complex top-level conditionals allows in-each
// to be used
Expand Down
51 changes: 51 additions & 0 deletions test/1.4/structure/T_top_hashif.dml
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
© 2024 Intel Corporation
SPDX-License-Identifier: MPL-2.0
*/
dml 1.4;

device test;

/// COMPILE-ONLY

#if (true) {
extern typedef int a_t;
typedef int b_t;
param p = 1;
} #else {
extern void *x;
import "nonexisting.dml";
param p = 2;
error;
}

#if (p != 1) { error; }

#if (false) {
typedef void *a_t;
extern typedef void *b_t;
error;
template t { error; }
} #else {
extern int x;
import "imported.dml";
template t { param from_t = true; }
}

#if (!imported) { error; }

is t;
#if (!from_t) { error; }

#if (dml_1_2 || false) {
error;
} #else {
#if (true) {
#if (!false) {
// nested top-level #ifs are permitted
param foo = true;
}
}
}

#if (!foo) { error; }
7 changes: 7 additions & 0 deletions test/1.4/structure/imported.dml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
/*
© 2024 Intel Corporation
SPDX-License-Identifier: MPL-2.0
*/
dml 1.4;

param imported = true;