Add --deprecate option for proxy ports and interfaces #209

mandolaerik · 2023-09-22T06:19:14Z

Shut up linter
Remove the --illegal-attrs flag
Remove --illegal-attrs flag
Remove --version, which was ignored
Migrate from optparse to argparse
Rename function
Refactor: Re-use common function
Factor out params for API version ranges
Add option for listing all warning tags
Add --deprecate=PORT_PROXY_IFACES
Add PORT_PROXY_ATTRS deprecation

mandolaerik · 2023-09-22T06:21:21Z

See also #208 where I extracted the uncontroversial parts, and #205 which presents an alternative approach.

mandolaerik · 2023-09-22T06:23:07Z

TODO:

extract documentation
convert --strict to deprecations
releasenote

mandolaerik · 2023-09-22T06:30:40Z

lib/1.4/dml-builtins.dml

+    #if (!_DEPRECATE_PORT_PROXY_IFACES && !_api_6_or_older) {
+        error "The PORT_PROXY_IFACES feature can only be enabled"
+            + " with API=6 or older";
+    }


I'm unsure about this part -- explicit guards in DML code is one of the things that I liked with #205; it makes the logic easy to explore and understand; however, I don't think this error statement is reachable (there is no flag to disable deprecations, and the default setting doesn't trigger the error). So this code just serves as (statically) executable documentation; it validates that the feature is indeed deprecated in APIs newer than 6. Unfortunately it doesn't validate a tight bound; the #if would not happen if 6 was the first deprecated version.

If you remember my --compat rant, I think we one day may trip into the situation where we will need to offer an option to revert a particular deprecation/semantic change. But those will be very particular situations, and the deprecation of proxy ports and interfaces certainly isn't one of them.
My opinion is that this only makes sense if we eventually plan to introduce a general --compat flag (as opposed to ad-hoc flags where we are selective about what kind of changes that users can turn off) -- I'm not particularly fond of a general --compat option, and I'm guessing you are even less so.
If we won't have a general --compat flag), there's no point to this. Why bother with executable documentation if you can just put a comment (here, or elsewhere?)

We do have a generic compat flag, namely --simics-api.

... where the meaning of --simics-api=7 is sealed the day 7 is released.

By "general" in this context I mean "granular and at will", e.g. --compat=PORT_PROXY_IFACES, like the opposite of --deprecate/--no-compat.

And by "ad-hoc" I mean if we ever make a change that we discover "oops, people have legit reasons to want to reverse this change and keep using modern Simics, but we still want the reversal of the change to be 'opt-in' and lock out your model from certain features if necessary as to not shackle future development of DMLC" then instead of adding a full-fledged --compat we instead let people say --enable-deprecated-[feature name]

Right, we can later choose to allow opting out from certain deprecations in API versions where they are enabled by default. If the need should arise.

Indeed; and the gist of my argument is that seeing how we're very unlikely to want --compat in favor of ad-hoc compatibility flags, I don't think we want this executable documentaiton, especially if it incurs an unneeded auto param (unless we do want people to access it, but then it shouldn't be _ prefixed`.)

I think the auto params are nice to have; in particular, good for workarounds in common code. The reason for the _ prefix is that they will be removed when they are obsolete.

mandolaerik · 2023-09-22T06:33:39Z

test/1.4/legacy/T_PORT_PROXY_IFACES_disabled.dml

No tests for 1.2. This is on purpose, the 1.2-specific code is just that auto params are declared, and if they wouldn't exist, then a minimal 1.2 file would fail.

syssimics · 2023-09-22T07:13:33Z

Verification #12227482: fail

py/dml/deprecations.py

mandolaerik · 2023-09-22T07:27:09Z

lib/1.4/dml-builtins.dml

+        error "The PORT_PROXY_IFACES feature can only be enabled"
+            + " with API=6 or older";
+    }
+    param _DEPRECATE_PORT_PROXY_ATTRS auto;


The naming scheme can be debated -- we don't use UPPER_SNAKE_CASE for other params, the uppercase is inherited from the command-line tags, which are uppercase for consistency with warning and error tags. I'm not sure that consistency is important; we could just lowercase deprecation tags. We could also mix-case, as in _deprecate_PORT_PROXY_ATTRS. Or just keep it uppercase; there is uncontroversial widespread use of uppercase param names, in particular for simulation-related constants or array size parameters.

My vote is mixed case, but I agree that whatever choice we pick is likely to be uncontroversial.

syssimics · 2023-09-22T11:06:14Z

Verification #12228488: pass

syssimics · 2023-09-22T13:06:39Z

Verification #12229149: pass

mandolaerik · 2023-09-22T19:50:19Z

Added first draft of documentation. Need to discuss terminology in more detail.

syssimics · 2023-09-22T19:53:30Z

Verification #12231545: fail

syssimics · 2023-09-22T20:22:00Z

Verification #12231565: pass

syssimics · 2023-09-24T15:39:37Z

Verification #12236973: fail

syssimics · 2023-09-24T17:00:01Z

Verification #12236979: pass

mandolaerik · 2023-09-26T17:29:15Z

The conversion of some dml12 features into compat tags also requires that the tests for these are rewritten with /// DMLC-FLAG --no-compat=xyz and/or /// DMLC-FLAG --simics-api=6

syssimics · 2023-09-26T17:31:53Z

Verification #12250481: pass

mandolaerik · 2023-09-26T18:29:29Z

Also, one non-apparent change in this PR is that the dml12_* compat features are now disabled in 7; this can possibly cause issues in our builds of 7. Need to do some test build. If there are problems, we can bump last_api_version of problematic features to 7 within this PR, and update models in separate PRs.

syssimics · 2023-09-27T07:55:10Z

Verification #12253365: fail

lwaern-intel

I quite like this. Some changes needed though.

doc/1.4/deprecations-header.md

lwaern-intel · 2023-09-27T08:23:45Z

doc/1.4/deprecations-header.md

+DMLC provides a command-line flag `--api-version` to specify the API version to
+be used for a model. When building with the standard Simics build system, this is controlled by the `SIMICS_API_VERSION` variable in `make`, or the `SIMICS_API`/`MODULE_SIMICS_API` variable in `CMake`.
+
+DMLC also provides the <code>--no-compat=_tag_</code> flag, which disables the


Wait, <code>the last word is _italicized_</code> works? Huh. In the ref manual we use <tt>the last work is <em>italicized</em></tt>. We should pick one for consistency.

I don't agree: I will not remember style at this level of detail so it will cost more than it gives. I can see two cases where markdown style matters:

we need to avoid markdown that relyies on corners that have non-local effect, such as an unescaped _underscore that works fine as long as no other underscore appears later in the same paragraph.

it's nice to ban other inconsistencies as long we can run a linter to enforce the rule.

lib/1.2/dml-builtins.dml

py/dml/compat.py

py/dml/structure.py

lwaern-intel · 2023-09-27T09:43:05Z

py/dml/globals.py

@@ -58,14 +60,13 @@
 # types.TypeSequence -> codegen.TypeSequenceInfo
 type_sequence_infos = {}

-# 1.4 style integer operations in 1.2, --strict-dml12-int
-strict_int_flag = None
+enabled_compat = set()


set() vs. Set()? It certainly won't affect generated C, but may affect, say, if we want to ever print out what compatibility tags that are enabled.

Then we should print sorted(enabled_compat)

test/1.4/legacy/port_proxy_attrs.dml

test/1.2/syntax/T_int.dml

py/dml/dmlc.py

syssimics · 2023-09-27T12:34:34Z

Verification #12254809: fail

This is nice because it allows comparisons like "simics_api < 7" to work also in Simics 10. We use the integer 4 to represent the API "4.8"

This also included some renames and change in polarities; in particular, all deprecated features are now disabled by default, as required by some unit tests.

Also flatten the compat.features dict and use string-only API representation in env.py

Triggered by `VPOP + VPOP` when compiling with --no-compat=dml12_int, or by 1.2/errors/EAPPLY when enabling --no-compat=dml12_int,dml12_misc

Passing --no-compat=dml12_misc triggers this in many tests.

syssimics · 2023-10-10T14:55:03Z

Verification #12324952: fail

Deprecating requires updating lots of tests and components; better to push that to a separate PR.

syssimics · 2023-10-10T16:22:57Z

Verification #12325435: pass

syssimics · 2023-10-10T17:34:01Z

Verification #12325946: fail

lwaern-intel

Looks good! Though I still don't like the _compat_ prefix of the autoparams.

lwaern-intel · 2023-10-11T08:14:44Z

py/dml/structure.py

+                        # code from 1.2 with --no-compat=dml12_misc
+                        dml.globals.dml_version != (1, 2)
+                        or p.site.dml_version == (1, 2)
+                        or p.site != sym.site):


OW
By the way, should we have some standard way to annotate hacks and todos? I've always used # HACK: and # TODO: because my editor highlights them.

lwaern-intel · 2023-10-11T08:16:45Z

py/dml/toplevel.py

+    if version != (1, 2):
+        for feature in [compat.dml12_inline, compat.dml12_not,
+                        compat.dml12_misc]:
+            dml.globals.enabled_compat.discard(feature)



It feels weird this is done separately from all other compatibility enablement logic. Though we want to do such logic as early as possible, and we have no choice to place this part of it right here.

Oh, and I notice that I forgot dml12_goto there! By sheer luck, this is not a problem since goto is forbidden by the grammar.

syssimics · 2023-10-11T13:00:56Z

Verification #12330620: pass

mandolaerik requested a review from lwaern-intel September 22, 2023 06:19

mandolaerik commented Sep 22, 2023

View reviewed changes

py/dml/deprecations.py Outdated Show resolved Hide resolved

mandolaerik commented Sep 22, 2023

View reviewed changes

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from d2688d9 to 631713b Compare September 22, 2023 10:37

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from 631713b to 37be595 Compare September 22, 2023 12:38

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from 05db6c6 to 78879d4 Compare September 22, 2023 19:54

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from 78879d4 to 769139e Compare September 24, 2023 15:18

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from 769139e to 4018e59 Compare September 24, 2023 16:32

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch 2 times, most recently from be9c393 to a4143e1 Compare September 26, 2023 17:03

lwaern-intel requested changes Sep 27, 2023

View reviewed changes

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from 2e3573e to 9972dd0 Compare September 27, 2023 11:45

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from 9972dd0 to 5dbb5b4 Compare September 27, 2023 14:18

mandolaerik added 17 commits October 10, 2023 16:27

Add --deprecate=PORT_PROXY_IFACES

df6f8da

Add PORT_PROXY_ATTRS deprecation

11c44c0

Add documentation for --deprecation

4313dcb

Convert --strict-dml12 to three deprecation tags

18ad914

Use integers to represent API versions

2a90ee1

This is nice because it allows comparisons like "simics_api < 7" to work also in Simics 10. We use the integer 4 to represent the API "4.8"

Change --deprecate to --no-compat

7ff5957

This also included some renames and change in polarities; in particular, all deprecated features are now disabled by default, as required by some unit tests.

Refactor: represent API versions with a dedicated class

82499b0

Also flatten the compat.features dict and use string-only API representation in env.py

Validate that dml-builtins declares all autoparams

6ed6910

Use compat system for --strict-int

4c0c05e

Use compat to control the default of use_io_memory param

2dad430

Use compat system for the obj param of ports/banks

20336a6

Repair odd ICE

d2867f0

Triggered by `VPOP + VPOP` when compiling with --no-compat=dml12_int, or by 1.2/errors/EAPPLY when enabling --no-compat=dml12_int,dml12_misc

Avoid errors on typeof $this in arrays of registers or fields

782a81f

Passing --no-compat=dml12_misc triggers this in many tests.

Avoid overflow with --no-compat=dml12_int

d06d43e

Add rudimentary tests for help flags

91e2424

Add release note for --no-compat flag

ae88ac5

Avoid incorrect ENAMECOLL with dml12_misc

bc55035

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from 8aed632 to 2b06464 Compare October 10, 2023 14:31

mandolaerik added 2 commits October 10, 2023 17:54

Extract goto to a separate compat feature

18febee

Temporarily bump last API of port proxy flags to 7

0019327

Deprecating requires updating lots of tests and components; better to push that to a separate PR.

mandolaerik force-pushed the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch from 2b06464 to 0019327 Compare October 10, 2023 15:54

mandolaerik requested a review from lwaern-intel October 11, 2023 06:03

lwaern-intel approved these changes Oct 11, 2023

View reviewed changes

mandolaerik merged commit f81232c into intel:main Oct 11, 2023
1 check passed

mandolaerik deleted the pr/add-deprecate-option-for-proxy-ports-and-interfaces branch October 11, 2023 15:48

lwaern-intel mentioned this pull request May 27, 2024

Type Equality/Compatibility Revamp #286

Merged

Add --deprecate option for proxy ports and interfaces #209

Add --deprecate option for proxy ports and interfaces #209

Conversation

mandolaerik commented Sep 22, 2023

mandolaerik commented Sep 22, 2023

mandolaerik commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwaern-intel Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syssimics commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syssimics commented Sep 22, 2023

syssimics commented Sep 22, 2023

mandolaerik commented Sep 22, 2023

syssimics commented Sep 22, 2023

syssimics commented Sep 22, 2023

syssimics commented Sep 24, 2023

syssimics commented Sep 24, 2023

mandolaerik commented Sep 26, 2023

syssimics commented Sep 26, 2023

mandolaerik commented Sep 26, 2023

syssimics commented Sep 27, 2023

lwaern-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syssimics commented Sep 27, 2023

syssimics commented Oct 10, 2023

syssimics commented Oct 10, 2023

syssimics commented Oct 10, 2023

lwaern-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syssimics commented Oct 11, 2023

lwaern-intel Sep 22, 2023 •

edited

Loading