Add a warning for large unrolled loops #234
Conversation
|
Verification #12467586: pass |
lwaern-intel
force-pushed
the
lw/unrolled-loop-warning
branch
from
08bda7f to
d6cbc1a
Compare
|
Verification #13501484: ✅ pass |
| 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 |
| 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)) |
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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', | |||
| information, see the documentation of `WBIGUNROLL` in the | ||
| [Messages](messages.html) section. |
There was a problem hiding this comment.
better with a direct link, add <a id= on the destination if needed
There was a problem hiding this comment.
Oh, seems that messages.html#dt:wbigunroll would work out-of-the-box
There was a problem hiding this comment.
whaaat
That's a thing? Wow.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| explicitly suppressed via `--no-warn=WBIGUNROLL`. In contrast, | ||
| `--warn=WBIGUNROLL` doesn't allow for `WBIGUNROLL` to be suppressed at | ||
| all.''' |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I should keep the mention that you can either use --warn=WBIGUNROLL or --no-compat=suppress_WBIGUNROLL.
Agreed
This is both a proper PR and also just a Jenkins Pretest target to figure out how bad the issue is on VP