-
Notifications
You must be signed in to change notification settings - Fork 162
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
Optimize true/false conditions when coding if-statements #2199
Conversation
|
||
// if the condition is 'true', ignore other branches of the if-statement | ||
if (TNUM_EXPR(cond) == T_TRUE_EXPR) { | ||
STATE(IntrIgnoring) = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I don't like about this PR is that I have to access IntrIgnoring
twice in src/code.c
; previously, there was no other access to IntrIgnoring
, IntrReturning
or IntrCoding
in src/code.c
. And only a handful of access to any of them outside of src/intrprt.c
(namely in src/gap.c
and src/read.c
).
But this is only a minor blemish, IMHO
Here is another reason why we may not want this, or at least not quite the way it is done in this PR: In the future, I would expect that we keep two copies of the body (which really is an abstract syntax tree of the function): one which represents a pristine copy of the original function (possibly even with comments retained?), and the other an optimized version, were code transformations have been applied, such as the if-statement optimizations in this PR, but also more (e.g. constant folding for things like But I think it'll be trivial to revert the changes in this PR in the future, or adapt them to, say, only suppress the warnings about the undefined globals but not actually ignore the code bodies. So, I am not overly concerned, but wanted to mention these thoughts for completeness. |
eba3edb
to
a06694c
Compare
We already had some code which "optimized" some if-statements with one or two conditions, where one or both were constant equal to `true` or `false`. We skipped branches whose condition was always `false`. However, we did not use the `IntrIgnoring` mechanism for that. Thus, such code would still trigger warnings if it contained undefined globals, e.g. gap> f:=function() if false then undefined_global(); fi; end;; Syntax warning: Unbound global variable f:=function() if false then undefined_global(); fi; end;; ^ This can be viewed as a feature, or as a bug, depending on view point. A reason to consider it a "bug" (or at least an undesirable feature) is when using global constants to only conditionally execute code, such as this: if IsHPCGAP then UNLOCK(lock); fi; This code is optimized away in regular GAP, but still triggers a warning during parsing. With this commit, we change how we deal with true/false conditions in if-statements, by making use of the `IntrIgnoring` mechanism. This avoids the warnings about globals in the examples above. It also has the side effect of being "better" at optimizing away branches of an if-statement which can never be executed.
I think we should merge this, although we should only add things (like this, and the earlier Also, we are already simplifying code anyway, and any future optimisation could perfectly well start from here, instead of from the original code. |
Short version: This PR turns
if true then ... else ... fi;
andif false then ... fi;
into something that's even closer to#if 1 ... #else ... #endif
and#if 0 ... #endif
, by making it ignore access to global variables inside theelse
resp. theif false
branches. This is useful when usingif IsHPCGAP
to add HPC-GAP specific functionality into code. As one minor example for that, look at thelib/system.g
change in this PR. Another example is that right now inmaster
, if you modifylib/type1.g
orlib/oper1.g
(forcing GAP to use them, not their C counter parts), you'll see a bunch of warnings about undefined globals. With this PR, these are gone.Personally, I find this highly useful, esp. when writing code involving
if IsHPCGAP
(so also in packages at some point). But of course one can also be critical of this change: It might cause one to miss bugs in code that one "temporarily" disabled usingif false then ...
. However, I'd argue that this is not a major problem; one will after all immediately get a warning once on changes this toif true then...
.Here is the full commit message:
We already had some code which "optimized" some if-statements
with one or two conditions, where one or both were constant equal
to
true
orfalse
. We skipped branches whose condition wasalways
false
. However, we did not use theIntrIgnoring
mechanismfor that. Thus, such code would still trigger warnings if it contained
undefined globals, e.g.
This can be viewed as a feature, or as a bug, depending on view point.
A reason to consider it a "bug" (or at least an undesirable feature)
is when using global constants to only conditionally execute code, such
as this:
This code is optimized away in regular GAP, but still triggers a warning
during parsing.
With this commit, we change how we deal with true/false conditions in
if-statements, by making use of the
IntrIgnoring
mechanism. This avoidsthe warnings about globals in the examples above. It also has the side
effect of being "better" at optimizing away branches of an if-statement
which can never be executed.