Discard reference -- SIMICS-21584 #237

lwaern-intel · 2023-11-09T12:36:05Z

I don't like the way the compatibility feature is implemented, but doing it at AST building level was the best I could do. Any attempt to try to do it at parser level fell apart.

syssimics · 2023-11-09T15:09:10Z

Verification #12479725: fail

syssimics · 2023-11-10T11:42:05Z

Verification #12484192: pass

syssimics · 2023-11-15T11:09:07Z

Verification #12510780: pass

py/dml/c_backend.py

mandolaerik · 2023-11-15T18:27:29Z

py/dml/codegen.py

+        # TODO/HACK: The discard ref is exposed like this to allow it to be as
+        # keyword-like as possible while still allowing it to be shadowed.
+        # Once we remove support for discard_ref_shadowing the discard ref
+        # should become a proper keyword and its codegen be done via dedicated
+        # dispatch
+        if name == '_' and tree.site.dml_version() != (1, 2):


This may not suffice as a reminder, since the comment is not adjacent to the code that will be touched when the compat is removed. One may argue that you mention discard_ref_shadowing in the comment so grep will find it, but you may consider a # when removing, remember to also fix .. comment near the enabled_compat-checking code.

In the test? Not next to the declaration of the compatibility feature itself?

I was thinking of the code in dmlparse that checks enabled_compat, but I suppose compat.py makes even more sense.

mandolaerik · 2023-11-15T18:31:28Z

py/dml/codegen.py

+            else:
+                init = eval_initializer(site, tgt.ctype(), src_ast, location,
+                                        scope, False)
+                name = 'tmp%d' % (i,)


_tmp%d? (because what if extern int tmp0(void); exists and the initializer of a subsequent assignment calls it)

oh, the problem was not introduced by you. Still valid but not blocking then.

I just want to note that any issue this could only possibly cause would be incredibly niche. In particular, this could only present an issue if -g is switched on, because otherwise the C variable name will get uniquified. (Notice that all codegen involved here uses scope and not lscope, so DML resolution will never take these temporary variables into account; the only possible issue is if the wrong thing happens at C level).

But yes; if -g is switched on, and the initializer references some C literal tmp0 (made accessible via extern), then this could present an issue. But it's rather far-fetched.

mandolaerik · 2023-11-15T18:34:41Z

py/dml/compat.py

+@feature
+class discard_ref_shadowing(CompatFeature):
+    '''This compatibility feature allows declarations (within methods or
+    objects) to be named '_'. This will cause the discard reference `_` to be


s/'_'/'\_'/, or backtick. Unescaped underscores in markdown are scary.

mandolaerik · 2023-11-15T18:38:07Z

py/dml/dmlparse.py

@@ -2632,6 +2659,12 @@ def ident(t):
 def ident(t):
    t[0] = t[1]

+def ident_enforce_not_discard_ref(site, ident):
+    if (str(ident) == '_'


str should be redundant?

Probably. I did it because reserved also does it.

oh, that's probably python2 legacy, the unicode vs str vs bytes discrepancy. Everything is strings now (unicode errors are caught in toplevel), so this can be removed in reserved too.

mandolaerik · 2023-11-15T18:41:51Z

py/dml/dmlparse.py

    t[0] = ast.foreach(site(t), t[2], t[5], t[7])

 @prod_dml14
 def hashforeach(t):
    'statement_except_hashif : HASHFOREACH ident IN LPAREN expression RPAREN statement'
+    ident_enforce_not_discard_ref(site(t, 2), t[2])


Lots of these calls. Perhaps easier to make a single call in the ident production rule? And create a separate ident_or_underscore production rule, which would be like ident but without that call?

Nope. I tried, but ran into reduce/reduce conflicts.

Oh, but there should be some middle path: There should be no reduce/reduce after HASHFOREACH, so there you can use ident, and in other cases (the start token of statement, I suppose) there will be reduce/reduce so for those can do ident_or_underscore with a manual ident_enforce_not_discard_ref. Could make sense if it's not a vast majority of all cases?

Such ad-hoc approaches would be very fragile to parser changes (i.e. a change could force rewriting an existing ident to ident_or_underscore) not to mention would introduce an inconsistency (either ident_or_underscore is used or a manual ident_enforce_not_discard_ref is.) This would only serve to make the parser even more difficult to understand. So I feel it'd be worse.

I agree that the ident_enforce_not_discard_ref is sub-par, and would welcome any superior approach which is at least as general and consistently applicable. ident_decl (i.e. ident with the check and warning) would be preferrable, if LALR didn't ruin our day.

I would argue the opposite wrt fragility: your current approach requires that any new rule that uses ident remembers to call ident_enforce_not_discard_ref, if not the deprecation will silently be incomplete.

But we probably just misunderstand each other. More efficient if I try to code up what I mean, perhaps I will understand better when I see the R/R conflicts myself.

I made an attempt, now pushed to this PR.

mandolaerik

Approved if you fix or dismiss remaining comments.

lwaern-intel · 2023-11-16T16:22:19Z

py/dml/ctree.py

+        rt = safe_realtype_shallow(typ)
+        # There is a reasonable implementation for this case (memcpy), but it
+        # never occurs today
+        assert not isinstance(typ, TArray)


Sloppy. should use rt instead, as should isinstance(typ, TExternStruct).

mandolaerik · 2023-11-16T20:20:25Z

test/1.4/expressions/T_discard_ref.dml

@@ -5,6 +5,8 @@
 dml 1.4;
 device test;

+/// DMLC-FLAG --no-compat=discard_ref_shadowing


Without this, the test passed in t126 before I added objident_or_underscore.

However, I suppose the error would still have been captured in the 7 build, so one may argue that adding the flag reduces test coverage in CI (the file is never compiled without the flag). So perhaps this commit should be dropped.

syssimics · 2023-11-16T20:21:27Z

Verification #12518623: fail

syssimics · 2023-11-16T20:24:36Z

Verification #12518616: pass

lwaern-intel · 2023-11-17T08:14:25Z

py/dml/dmlparse.py

-    if compat.discard_ref_shadowing not in dml.globals.enabled_compat:
+    if (compat.discard_ref_shadowing not in dml.globals.enabled_compat
+        # forgive the `param _ auto` declaration
+        and not site(t).filename().endswith('dml-builtins.dml'):


we don't have a param _ auto declarqtion!

oh, sorry, I was so sure we had that I didn't even bother to check. So the top commit is rubbish, now removed.

lwaern-intel · 2023-11-17T08:17:01Z

py/dml/dmlparse.py

+    raise ESYNTAX(site(t, 1), str(t[1]), "reserved word")
+
+@prod_dml14
+@lex.TOKEN(ident_rule('ident', ['_']))


just do

ident : _

also in my WIP I called the token DISCARDREF.

_ follows the convention that identifier-formed tokens use that as their name.

And thanks for spotting my stupid copy-paste

Allowed for method-local bindings and index variables. Method-local bindings named '_' are not added to scope Index variables named '_' do not get a parameter created for them

syssimics · 2023-11-30T12:39:03Z

Verification #12598423: fail

syssimics · 2023-11-30T12:45:58Z

Verification #12598439: fail

mandolaerik · 2023-12-02T07:12:26Z

doc/1.4/language.md

+
+"\_" may be used as an identifier for local variables, as well as other
+method-local bindings such as the method parameters, and the bound identifier
+in `foreach`/`#foreach`/`#select` statements, and message component paramters of


s/paramters/parameters

mandolaerik

Looks good. Two remarks:

I would vote for also adding struct memebers named _. This can just as well be added in a separate PR, though.
Looks like you preserve old behaviour in 1.2, thus introducing a discrepancy between 1.2 and 1.4. Doing the same in 1.2 and 1.4 would possibly give less code, but OTOH this doesn't really matter (dying code) and the code is already written.

lwaern-intel · 2023-12-02T11:05:04Z

py/dml/symtab.py

@@ -91,6 +91,9 @@ def lookup(self, name, local = False):
    def add(self, sym):
        if not isinstance(sym, Symbol):
            raise TypeError(repr(sym) + " is not a Symbol")
+        if (sym.name == '_' and sym.site is not None
+            and sym.site.dml_version() != (1, 2)):
+            raise ICE(sym.site, "Symbol with name '_' added to Symbtab")


lwaern-intel requested a review from mandolaerik November 9, 2023 12:36

lwaern-intel force-pushed the lw/21584 branch from 1be5b36 to ea9f269 Compare November 10, 2023 10:59

lwaern-intel mentioned this pull request Nov 15, 2023

Store all permutations in a global shared array, so that one 32-bit index per register is sufficient. #239

Closed

lwaern-intel force-pushed the lw/21584 branch from ea9f269 to 9c1b160 Compare November 15, 2023 10:42

lwaern-intel mentioned this pull request Nov 15, 2023

Add DMLC option to not suppress GCC's Wunused for local variables -- SIMICS-21585 #240

Closed

mandolaerik reviewed Nov 15, 2023

View reviewed changes

py/dml/c_backend.py Show resolved Hide resolved

mandolaerik reviewed Nov 15, 2023

View reviewed changes

mandolaerik approved these changes Nov 15, 2023

View reviewed changes

lwaern-intel commented Nov 16, 2023

View reviewed changes

mandolaerik reviewed Nov 16, 2023

View reviewed changes

lwaern-intel commented Nov 17, 2023

View reviewed changes

mandolaerik force-pushed the lw/21584 branch 2 times, most recently from 8cfdcb9 to 9c1b160 Compare November 17, 2023 09:12

lwaern-intel added 3 commits November 30, 2023 12:07

Add Noop, a better variant of Null for internal usages

061553a

Port of RAII's writable/addressable/c_lval revamp

6a22da5

Port of RAII's Expression.write/Initializer.assign_to revamp

480dbcf

lwaern-intel force-pushed the lw/21584 branch from 9c1b160 to d56cfe8 Compare November 30, 2023 12:14

lwaern-intel added 3 commits November 30, 2023 13:18

Discard reference -- SIMICS-21584

dce32e4

Codegen the discard reference via dedicated AST dispatch

d940bfa

Support '_' as a magic identifier for certain declarations

58b51d9

Allowed for method-local bindings and index variables. Method-local bindings named '_' are not added to scope Index variables named '_' do not get a parameter created for them

Check for duplicate parameters in shared methods

7fe389c

lwaern-intel force-pushed the lw/21584 branch from d56cfe8 to 7fe389c Compare November 30, 2023 12:18

mandolaerik reviewed Dec 2, 2023

View reviewed changes

mandolaerik approved these changes Dec 2, 2023

View reviewed changes

lwaern-intel commented Dec 2, 2023

View reviewed changes

Discard reference -- SIMICS-21584 #237

Are you sure you want to change the base?

Discard reference -- SIMICS-21584 #237

Conversation

lwaern-intel commented Nov 9, 2023 • edited Loading

syssimics commented Nov 9, 2023

syssimics commented Nov 10, 2023

syssimics commented Nov 15, 2023

Choose a reason for hiding this comment

lwaern-intel Nov 16, 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

lwaern-intel Nov 16, 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

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 Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandolaerik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syssimics commented Nov 16, 2023

syssimics commented Nov 16, 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

syssimics commented Nov 30, 2023

syssimics commented Nov 30, 2023

Choose a reason for hiding this comment

mandolaerik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lwaern-intel commented Nov 9, 2023 •

edited

Loading

lwaern-intel Nov 16, 2023 •

edited

Loading

lwaern-intel Nov 16, 2023 •

edited

Loading

lwaern-intel Nov 16, 2023 •

edited

Loading