Skip to content
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

Add a warning for large unrolled loops #234

Open
wants to merge 2 commits 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
7 changes: 7 additions & 0 deletions RELEASENOTES.docu
Original file line number Diff line number Diff line change
Expand Up @@ -1795,4 +1795,11 @@ extern typedef struct { } my_type_t;</pre> </add-note></build-id>
<build-id _6="next" _7="next"><add-note>Multiple `extern` declarations for the
same identifier are now allowed as long as they all declare the same type
for the identifier <bug number="SIMICS-22472"/>.</add-note></build-id>
<build-id _6="next" _7="next"><add-note> Added the warning <tt>WBIGUNROLL</tt>
which is emitted when a <tt>#select</tt> or <tt>#foreach</tt> statement is
compiled to a large unrolled loop.
This warning is only enabled by default with Simics API version 8 or
above. With version 7 and below it must be explicitly enabledby passing
Copy link
Contributor

Choose a reason for hiding this comment

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

enabled by

either <tt>--no-compat=suppress_WBIGUNROLL</tt> or
<tt>--warn=WBIGUNROLL</tt> to DMLC.</add-note></build-id>
</rn>
1 change: 1 addition & 0 deletions lib/1.2/dml-builtins.dml
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ template device {
parameter _compat_io_memory auto;
parameter _compat_shared_logs_on_device auto;
parameter _compat_suppress_WLOGMIXUP auto;
parameter _compat_suppress_WBIGUNROLL auto;
parameter _compat_legacy_attributes auto;
parameter _compat_lenient_typechecking auto;
parameter _compat_dml12_inline auto;
Expand Down
1 change: 1 addition & 0 deletions lib/1.4/dml-builtins.dml
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,7 @@ template device {
param _compat_io_memory auto;
param _compat_shared_logs_on_device auto;
param _compat_suppress_WLOGMIXUP auto;
param _compat_suppress_WBIGUNROLL auto;
param _compat_legacy_attributes auto;
param _compat_lenient_typechecking auto;
param _compat_dml12_inline auto;
Expand Down
11 changes: 11 additions & 0 deletions py/dml/codegen.py
Original file line number Diff line number Diff line change
Expand Up @@ -3031,12 +3031,19 @@ def stmt_select(stmt, location, scope):
else_dead = True
break

iterations = len(clauses)
if else_dead:
(last_cond, last_stmt) = clauses.pop(-1)
assert last_cond.constant and last_cond.value
if_chain = last_stmt
else:
if_chain = codegen_statement(else_ast, location, scope)
if iterations > WBIGUNROLL.limit and isinstance(lst, List):
report(WBIGUNROLL(stmt.site,
'#'*(stmt.site.dml_version() != (1, 2))
Copy link
Contributor

Choose a reason for hiding this comment

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

space around binop

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a stylistic choice we can quibble about: when combining + and *, I actually prefer omitting spaces around *; I feel that increases readability re. operator precedence. I see it as similar to implicit multiplication in math via concatenation (2(3)).

Copy link
Contributor

Choose a reason for hiding this comment

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

Here we don't combine + and *. but we do combine * and !=.

And I agree that PEP-8 is not always optimal; in particular, I find f(foo=bar == 2) a bit weird. But I just accept and obey the style for consistency, unless there's strong reasons not to.

Copy link
Contributor Author

@lwaern-intel lwaern-intel Jun 17, 2024

Choose a reason for hiding this comment

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

Here we don't combine + and *

I mean... we are, though the + is on a new line. That's good enough for readability, I admit. Though that doesn't apply to the below '#'*(site.dml_version() != (1, 2)) + 'foreach',

we do combine * and !=`.

Not on the same syntactic level, the != is within an (); I'm talking about * and +, which is on the same syntactic level, and where operator precedence determines how associations are made. This is where I'm arguing that omitting spaces around * such that such parts are more obviously whole terms helps readability. To exemplify, compare

   bla_bla_blau * blib * blub_blub + glabradahadaba + blara * dabara
+ canyoutellthat * ihavegivenup + on * variable * naming

vs.

   bla_bla_blau*blib*blub_blub + glabradahadaba + blara*dabara
+ canyoutellthat*ihavegivenupon + on*variable*naming

And that does apply to '#'*(site.dml_version() != (1, 2)) + 'foreach', but not as strongly. If you write out:

'#' * (site.dml_version() != (1, 2)) + 'foreach'

It's not as obviously clear at a glance that the (site.dml_version() != (1, 2)) is used for controlling a term of the string concatenation sequence, as opposed to being a term in of itself. Of course, the counterargument to that would be parentheses...:

('#' * (site.dml_version() != (1, 2))) + 'foreach'

If you insist on spacing, I would do the above. even though i kinda hate it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't see the + because of how GH crops things. Still, I find it strange to break the spacing rule when you have a binop within an operand where you do obey the spacing rule.

If you ask me what I prefer, then it's the normal python idiom for expressing when a value depends on a condition:

('' if version == (1, 2) else '#') + 'foreach'

I quite strongly dislike the use of bools for arithmetic on iterables. But I tolerate it since it doesn't break any agreed-upon style.

+ 'select',
iterations))

for (cond, stmt) in reversed(clauses):
if_chain = mkIf(cond.site, cond, stmt, if_chain)
return [if_chain]
Expand Down Expand Up @@ -3162,6 +3169,10 @@ def foreach_constant_list(site, itername, lst, statement, location, scope):
stmt)
spec.append(mkCompound(site, decls + [stmt]))

if len(spec) > WBIGUNROLL.limit and isinstance(lst, List):
report(WBIGUNROLL(site,
'#'*(site.dml_version() != (1, 2)) + 'foreach',
Copy link
Contributor

Choose a reason for hiding this comment

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

space around binop

len(spec)))
return [mkUnrolledLoop(site, spec,
context.label if context.used else None)]

Expand Down
24 changes: 24 additions & 0 deletions py/dml/compat.py
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,30 @@ class suppress_WLOGMIXUP(CompatFeature):
short = "Suppress the warning 'WLOGMIXUP' by default"
last_api_version = api_6

@feature
class suppress_WBIGUNROLL(CompatFeature):
'''This compatibility feature makes it so the warning `WBIGUNROLL` is
suppressed by default. `WBIGUNROLL` warns about `#foreach` and `#select`
statements that compile to large unrolled loops &mdash; for more
information, see the documentation of `WBIGUNROLL` in the
[Messages](messages.html) section.
Comment on lines +206 to +207
Copy link
Contributor

Choose a reason for hiding this comment

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

better with a direct link, add <a id= on the destination if needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, seems that messages.html#dt:wbigunroll would work out-of-the-box

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whaaat
That's a thing? Wow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Though validate_md_links.py hates it. I guess we could patch it to tolerate #dt:blabla if any line in the target file starts with * blabla?

Copy link
Contributor Author

@lwaern-intel lwaern-intel Jun 17, 2024

Choose a reason for hiding this comment

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

Oh, seems that messages.html#dt:wbigunroll would work out-of-the-box

wait. What experiment did you do, that made you say this?
If we look at what messages_to_md.py does...

        f.write(f"  <dt><b>\n\n{fmt_message(m)} [{m.__name__}]</b></dt>\n")

Looking at this, I don't understand why the hyperlink syntax would pick out and work specifically with m.__name__.

If this doesn't work, maybe the better approach is to just tweak messages_to_md.py to generate those anchors, so we don't have to hand-write them.

Copy link
Contributor

@mandolaerik mandolaerik Jun 17, 2024

Choose a reason for hiding this comment

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

What experiment did you do, that made you say this?

I looked at the generated messages.html, and also typed file:///path/to/messages.html#dt:wsystemc into my web browser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... huh!


`WBIGUNROLL` is suppressed by default below Simics API version 8 in order
to avoid overwhelming users with warnings. Addressing occurences of large
unrolled loops should be done before or as part of migration to Simics API
version 8.

Passing `--no-compat=suppress_WBUGUNROLL` to DMLC has almost the same effect
as passing `--warn=WBIGUNROLL`; either will cause DMLC to report the warning
even when the Simics API version in use is below 8. The only difference
between these two options is that if `--no-compat=suppress_WBIGUNROLL` is
used (and `--warn=WBIGUNROLL` is not), then `WBIGUNROLL` may still be
explicitly suppressed via `--no-warn=WBIGUNROLL`. In contrast,
`--warn=WBIGUNROLL` doesn't allow for `WBIGUNROLL` to be suppressed at
all.'''
Comment on lines +219 to +221
Copy link
Contributor

Choose a reason for hiding this comment

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

that's somewhat weird, when you pass --X --no-X to a compiler, you expect that the one that appears last takes precedence.

In any case, I think we can drop this paragraph entirely; there is no essential difference and the main reason for a compat feature is for consistency with other things that change with API 8.

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 agree with dropping the explanation of the difference; that was inherited from the description of suppress_WLOGMIXUP, but it doesn't bear repeating. Though, I think I should keep the mention that you can either use --warn=WBIGUNROLL or --no-compat=suppress_WBIGUNROLL.

Copy link
Contributor

Choose a reason for hiding this comment

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

I should keep the mention that you can either use --warn=WBIGUNROLL or --no-compat=suppress_WBIGUNROLL.

Agreed

short = "Suppress the warning 'WBIGUNROLL' by default"
last_api_version = api_7

@feature
class legacy_attributes(CompatFeature):
'''This compatibility feature makes DMLC register all attributes using the
Expand Down
2 changes: 2 additions & 0 deletions py/dml/dmlc.py
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,8 @@ def main(argv):

if compat.suppress_WLOGMIXUP in dml.globals.enabled_compat:
ignore_warning('WLOGMIXUP')
if compat.suppress_WBIGUNROLL in dml.globals.enabled_compat:
ignore_warning('WBIGUNROLL')

for w in options.disabled_warnings:
if not is_warning_tag(w):
Expand Down
26 changes: 26 additions & 0 deletions py/dml/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -2291,6 +2291,32 @@ class WHOOKSEND(DMLWarning):
+ "Declarations section in the DML 1.4 reference manual for "
+ "information about the differences between 'send' and 'send_now'")

class WBIGUNROLL(DMLWarning):
limit = 64
__doc__ = f"""
A `#select` or `#foreach` statement was specified which caused DMLC to
study the body and generate duplicated C code for it a large number of
times (more than {limit} times.) This can dramatically increase both DMLC
and GCC compile times and code size, if not crash DMLC outright.

To address this, you have two options:
* Ensure most iterations can be entirely eliminated at compile-time by
DMLC. For `#select`, this can be done by ensuring that the `where` check
will be a constant value (e.g. a constant equality) for most if not all
items of the specified list. For `#foreach`, encase the body in an `#if`
check that is false for most items of the specified list.
* Don't use `#select` or `#foreach`. Represent the specified compile-time
list instead as a `session` array or an `extern`ed C array, and iterate
through it using traditional `for` loops. For more information, see [the
various answers to this Stack Overflow question.](https://stackoverflow.com/questions/75073681/cannot-use-variable-index-in-a-constant-list)

This warning is only enabled by default with Simics API version 8 or above
(due to the compatibility feature `suppress_WBIGUNROLL`.)
"""
fmt = ("This '%s' statement compiles to an unrolled loop where the loop "
+ "body is studied and generated %s times. This may dramatically "
+ "increase both DMLC and GCC compilation times, if not crash DMLC "
+ "outright!")

class PSHA1(PortingMessage):
"""The `port-dml` script requires that the DML file has not been
Expand Down
15 changes: 15 additions & 0 deletions test/1.2/legacy/T_suppress_WBIGUNROLL_disabled.dml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
© 2024 Intel Corporation
SPDX-License-Identifier: MPL-2.0
*/
dml 1.2;

device test;

/// COMPILE-ONLY
/// NO-CC

/// DMLC-FLAG --no-compat=suppress_WBIGUNROLL

/// SCAN-FOR-TAGS suppress_WBIGUNROLL.dml
import "suppress_WBIGUNROLL.dml";
14 changes: 14 additions & 0 deletions test/1.2/legacy/T_suppress_WBIGUNROLL_enabled.dml
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
© 2024 Intel Corporation
SPDX-License-Identifier: MPL-2.0
*/
dml 1.2;

device test;

/// COMPILE-ONLY
/// NO-CC

/// DMLC-FLAG --simics-api=7

import "suppress_WBIGUNROLL.dml";
80 changes: 80 additions & 0 deletions test/1.2/legacy/suppress_WBIGUNROLL.dml
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/*
© 2024 Intel Corporation
SPDX-License-Identifier: MPL-2.0
*/
dml 1.2;

mandolaerik marked this conversation as resolved.
Show resolved Hide resolved
// expectations in this file are selectively enabled using SCAN-FOR-TAGS

data int zero = 0;

parameter zeroes_64 = [$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero];

// Sixtyfourth zero is constant
parameter zeroes_65 = [$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, $zero,
$zero, $zero, $zero, $zero, $zero, $zero, $zero, 0,
$zero];

bank b {
register regs[i in 0..64] size 4 @ undefined;
}

method init {
local int nonconstant;

// no warning
foreach x in ($zeroes_64) assert true;
// The else branch is not considered an iteration
select x in ($zeroes_64) where (x == nonconstant) {
assert true;
} else assert true;

/// WARNING WBIGUNROLL
foreach x in ($zeroes_65) assert true;
/// WARNING WBIGUNROLL
select x in ($zeroes_65) where (x == nonconstant) {
assert true;
} else assert true;

// no warning
foreach x in ($zeroes_65) {
// As sixtyfourth bit is constant 0, DML 1.2 semantics eliminates this
// if, and subsequently causes that iteration to be omitted from
// codegen entirely; reducing the total count to 64
if (x != 0) assert true;
}

// As sixtyfourth bit is constant 0, select can cut short at it, reducing
// the total number of iterations to 64
select x in ($zeroes_65) where (x == 0) {
assert true;
} else assert true;

// As sixtyfourth bit is constant 0, select can omit the check and thus
// iteration for it, reducing the total number of iterations to 64
select x in ($zeroes_65) where (x != 0) {
assert true;
} else assert true;


// WBIGUNROLL is only reported for compile-time lists defined via list
// syntax, not the object lists generated by DMLC
// no warning
foreach x in ($b.unmapped_registers) assert true;
select x in ($b.unmapped_registers) where (x == nonconstant) {
assert true;
} else assert true;
}