Skip to content

Create provisional and compat features around the vect type #328

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

Merged
merged 10 commits into from
Jun 26, 2025

Conversation

mandolaerik
Copy link
Contributor

@mandolaerik mandolaerik commented Oct 21, 2024

  • Include docs for compat and provisionals in 1.2 refman too
  • Allow provisional declarations in DML 1.2
  • Repair pickling of provisionals
  • Create provisional and compat features around the vect type
  • Dogfood the c_vect provisional

@mandolaerik mandolaerik changed the title Dogfood the c_vect provisional Create provisional and compat features around the vect type Oct 21, 2024
@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from 6e31f31 to 1d81bd5 Compare October 21, 2024 12:02
@syssimics
Copy link
Contributor

Verification #14005157: ❌ fail

@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from 1d81bd5 to b2a4cfd Compare October 22, 2024 10:57
@syssimics
Copy link
Contributor

Verification #14009292: ❌ fail

@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from b2a4cfd to c587b73 Compare October 22, 2024 12:37
@syssimics
Copy link
Contributor

Verification #14009657: ❌ fail

@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from c587b73 to 015229f Compare October 22, 2024 15:45
@syssimics
Copy link
Contributor

Verification #14010442: ❌ fail

@syssimics
Copy link
Contributor

Verification #14010884: ✅ pass

The duplicated headers are unfortunate, and is there because one
of them mentions "1.4" specifically. It's not a problem if we let
the 1.2 versions rot.
@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from 72156e4 to a4065c4 Compare June 25, 2025 21:37
@@ -169,9 +170,12 @@ template write_only {
log spec_viol, log_level, Register_Read:
"Read from write-only register %s (returning 0).", $qname;
log_level = 2;
} else {
} else if ($dev._compat_warning_statement) {
_warning "The write_only template only makes sense on registers."
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get why this works. Why don't you get ESYNTAX on this when the compat feature is disabled? #if shouldn't stop the AST from being built. Or... does it??!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, it's because we suppress the warning while building dml-builtins.dmlast. After that the parser will not look at dml-builtins.dml again.

Copy link
Contributor

@lwaern-intel lwaern-intel Jun 26, 2025

Choose a reason for hiding this comment

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

Oh good lord. So you're saying this code is only meaningful because we can leverage dmlast. Let me see if I get this right:

  • The .dmlast is prebuilt, and it is so with the compat feature enabled. Because of that, it's able to use the _warning statement without provoking ESYNTAX, no matter if the compat feature is enabled/disabled for the individual device.
  • But the value of $dev._compat_warning_statement is resolved only when it's time to actually build the device. Because of that, if the individual device does not have that compat feature enabled, then compilation will end up leveraging error instead of _warning .

Is that correct? That's...
I...
I don't like that. I don't like that we have code in dml-builtins/utility that only works because of dmlast caching; because we're able to parse and build the AST using a different set of flags compared to when building the device. That feels extremely bad.
Not to mention, people (at least I) look towards dml-builtins/utility for reference and inspiration -- this code snippet could leave people thinking that the pattern

#if ($dev._compat_warning_statement) {
    _warning "...";
}

would work in general, when the fact is it only works due to extremely niche circumstances. Worse yet, they may extrapolate to expect that pattern would work for other syntactical features controlled by a compatibility feature.

Copy link
Contributor

@lwaern-intel lwaern-intel Jun 26, 2025

Choose a reason for hiding this comment

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

You must at the very least add a comment about this. This should be treated as a HACK. A pretty egregious one at that -- you would never expect the correctness of a piece of model code to be dependent on whether or not it gets cached.

Copy link
Contributor Author

@mandolaerik mandolaerik Jun 26, 2025

Choose a reason for hiding this comment

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

Sure, I can add a comment.

Not to mention, people (at least I) look towards dml-builtins/utility for reference and inspiration

If you are in search of inspiration, then visiting to DML 1.2 code is strongly discouraged. Not good for your health.

(I would also have been concerned about the hackiness of this code if this wasn't dying code)

}
```

* DML natively supports `vect` types in `foreach` statements.
Copy link
Contributor

Choose a reason for hiding this comment

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

Only true in DML 1.2!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a test to prove you wrong

Copy link
Contributor

Choose a reason for hiding this comment

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

Added a comment to prove you proving me wrong, wrong!

@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from a4065c4 to a4864cf Compare June 25, 2025 22:12
@mandolaerik mandolaerik requested a review from lwaern-intel June 25, 2025 22:13
@syssimics
Copy link
Contributor

PR Verification: ✅ success

@syssimics
Copy link
Contributor

PR Verification: ❌ failure

@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from a4864cf to acea79a Compare June 26, 2025 05:45
@syssimics
Copy link
Contributor

PR Verification: ✅ success

@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from acea79a to 70d4d0e Compare June 26, 2025 09:19
@syssimics syssimics requested a review from lwaern-intel June 26, 2025 09:19
Copy link
Contributor

@lwaern-intel lwaern-intel left a comment

Choose a reason for hiding this comment

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

See my existential crisis re. abusing .dmlast caching

@syssimics
Copy link
Contributor

PR Verification: ✅ success

@mandolaerik mandolaerik force-pushed the pr/dogfood-the-c_vect-provisional branch from 70d4d0e to 8e97202 Compare June 26, 2025 10:17
@syssimics syssimics requested a review from lwaern-intel June 26, 2025 10:18
Copy link
Contributor

@lwaern-intel lwaern-intel left a comment

Choose a reason for hiding this comment

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

Approved modulo verification. Though I do feel inclined to bikeshed a bit.

error unless the [`experimental_vect` compatibility
feature](deprecations-auto.html#experimental_vect) is enabled.
'''
short = "Allow vect syntax based on the VECT macro"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm inclined to bikeshed this short desc a bit, for multiple reasons:

  • To make it more obvious as to what the syntax actually is and does (it's possibly to read this as the DML syntax also being VECT)
  • To explicitly state the connection to the Simics API
  • And to somewhat future-proof the formulation against the introduction of RAII vectors

My suggestion would be:

Enable 'vect' type syntax that compile to usages of the Simics 'VECT' macro

However, I don't insist on any on this, and this is not blocking.

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 prefer to leave disambiguation to the full docs. I guess most of the time when you look at the list it's one of the other features you are looking for, which means the main use case for the short desc is to quickly convince you that this is not the thing you are looking for. So better to be terse and simple. If you don't know what the VECT macro is about, then you don't use the feature and the short desc is not for you.

@syssimics
Copy link
Contributor

PR Verification: ✅ success

@mandolaerik mandolaerik merged commit 2f0f7b1 into intel:main Jun 26, 2025
@mandolaerik mandolaerik deleted the pr/dogfood-the-c_vect-provisional branch June 26, 2025 11:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants