From a06694c409b7201314f68c9eae18f5394cd16e8d Mon Sep 17 00:00:00 2001 From: Max Horn Date: Thu, 22 Feb 2018 11:36:47 +0100 Subject: [PATCH] Optimize true/false conditions when coding if-statements 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. --- lib/system.g | 12 --- src/code.c | 81 ++++++++-------- src/code.h | 2 +- src/intrprtr.c | 36 ++++---- src/intrprtr.h | 2 +- src/read.c | 9 +- tst/testbugfix/2017-09-07-elseif.tst | 24 ----- tst/testinstall/coding.tst | 133 +++++++++++++++++++++++++++ tst/testinstall/constant.tst | 17 +--- 9 files changed, 194 insertions(+), 122 deletions(-) delete mode 100644 tst/testbugfix/2017-09-07-elseif.tst create mode 100644 tst/testinstall/coding.tst diff --git a/lib/system.g b/lib/system.g index 5930d167ea..ca0cbe72a2 100644 --- a/lib/system.g +++ b/lib/system.g @@ -168,11 +168,6 @@ fi; ## For HPC-GAP, we want GAPInfo and its members to be accessible from all ## threads, so make members atomic or immutable. ## -if not IsHPCGAP then - # HACK to silence "Unbound global variable" warnings - AtomicList := fail; - AtomicRecord := fail; -fi; if IsHPCGAP then MakeReadWriteGVar("GAPInfo"); GAPInfo := AtomicRecord(GAPInfo); @@ -475,13 +470,6 @@ CallAndInstallPostRestore( function() end ); -if not IsHPCGAP then - # undo HACK to silence "Unbound global variable" warnings - Unbind(AtomicList); - Unbind(AtomicRecord); -fi; - - ############################################################################# ## #V GAPInfo.TestData diff --git a/src/code.c b/src/code.c index 0b31b773f4..b106fbebde 100644 --- a/src/code.c +++ b/src/code.c @@ -921,13 +921,34 @@ void CodeIfElse ( void ) void CodeIfBeginBody ( void ) { + // get and check the condition + Expr cond = PopExpr(); + + // if the condition is 'false', ignore the body + if (TNUM_EXPR(cond) == T_FALSE_EXPR) { + STATE(IntrIgnoring) = 1; + } + else { + // put the condition expression back on the stack + PushExpr(cond); + } } -void CodeIfEndBody ( +Int CodeIfEndBody ( UInt nr ) { /* collect the statements in a statement sequence if necessary */ PushStat( PopSeqStat( nr ) ); + + // get and check the condition + Expr cond = PopExpr(); + PushExpr(cond); + + // if the condition is 'true', ignore other branches of the if-statement + if (TNUM_EXPR(cond) == T_TRUE_EXPR) { + STATE(IntrIgnoring) = 1; + } + return 1; } void CodeIfEnd ( @@ -937,51 +958,23 @@ void CodeIfEnd ( Expr cond; /* condition of a branch */ UInt hase; /* has else branch */ UInt i; /* loop variable */ - Expr cond1 = 0; /* first condition */ - Expr cond2 = 0; /* second condition */ - - /* peek at the last two conditions */ - cond1 = PopExpr(); - hase = (TNUM_EXPR(cond1) == T_TRUE_EXPR); - if (nr == 2) { - cond2 = PopExpr(); - PushExpr(cond2); - } - PushExpr(cond1); - - // Some optimisation cases - if (nr == 1) { - if (TNUM_EXPR(cond1) == T_TRUE_EXPR) { - // Leave statement - PopExpr(); - return; - } - else if (TNUM_EXPR(cond1) == T_FALSE_EXPR) { - // Remove entire if statement - PopStat(); - PopExpr(); - PushStat(NewStat(T_EMPTY, 0)); - return; - } + + // if all conditions were false, the if-statement is an empty statement + if (nr == 0) { + PushStat(NewStat(T_EMPTY, 0)); + return; } - if (nr == 2 && hase) { - if (TNUM_EXPR(cond2) == T_TRUE_EXPR) { - // Leave 'true' case - PopStat(); - PopExpr(); - PopExpr(); - return; - } - else if (TNUM_EXPR(cond2) == T_FALSE_EXPR) { - // Leave 'false' case - Stat body = PopStat(); - PopExpr(); - PopStat(); - PopExpr(); - PushStat(body); - return; - } + // peek at the last condition + cond = PopExpr(); + hase = (TNUM_EXPR(cond) == T_TRUE_EXPR); + PushExpr(cond); + + // optimize 'if true then BODY; fi;' to just 'BODY;' + if (nr == 1 && hase) { + // drop the condition expression, leave the body statement + PopExpr(); + return; } /* allocate the if-statement */ diff --git a/src/code.h b/src/code.h index ecba7f6528..717e350e7a 100644 --- a/src/code.h +++ b/src/code.h @@ -692,7 +692,7 @@ extern void CodeIfElse ( void ); extern void CodeIfBeginBody ( void ); -extern void CodeIfEndBody ( +extern Int CodeIfEndBody ( UInt nr ); extern void CodeIfEnd ( diff --git a/src/intrprtr.c b/src/intrprtr.c index eb11340b5c..47936fbe89 100644 --- a/src/intrprtr.c +++ b/src/intrprtr.c @@ -547,6 +547,13 @@ void IntrIfBegin ( void ) { /* ignore or code */ if ( STATE(IntrReturning) > 0 ) { return; } + + // if IntrIgnoring is positive, increment it, as IntrIgnoring == 1 has a + // special meaning when parsing if-statements -- it is used to skip + // interpreting or coding branches of the if-statement which never will be + // executed, either because a previous branch is always executed (i.e., it + // has a 'true' condition), or else because the current branch has a + // 'false' condition if ( STATE(IntrIgnoring) > 0 ) { STATE(IntrIgnoring)++; return; } if ( STATE(IntrCoding) > 0 ) { CodeIfBegin(); return; } @@ -597,22 +604,15 @@ void IntrIfBeginBody ( void ) } } -void IntrIfEndBody ( +Int IntrIfEndBody ( UInt nr ) { UInt i; /* loop variable */ /* ignore or code */ - if ( STATE(IntrReturning) > 0 ) { return; } - if ( STATE(IntrIgnoring) > 1 ) { STATE(IntrIgnoring)--; return; } - if ( STATE(IntrCoding) > 0 ) { CodeIfEndBody( nr ); return; } - - - /* if the condition was 'false', the body was ignored */ - if ( STATE(IntrIgnoring) == 1 ) { - STATE(IntrIgnoring) = 0; - return; - } + if ( STATE(IntrReturning) > 0 ) { return 0; } + if ( STATE(IntrIgnoring) > 0 ) { STATE(IntrIgnoring)--; return 0; } + if ( STATE(IntrCoding) > 0 ) { return CodeIfEndBody( nr ); } /* otherwise drop the values for the statements executed in the body */ for ( i = nr; 1 <= i; i-- ) { @@ -621,6 +621,8 @@ void IntrIfEndBody ( /* one branch of the if-statement was executed, ignore the others */ STATE(IntrIgnoring) = 1; + + return 1; } void IntrIfEnd ( @@ -629,19 +631,15 @@ void IntrIfEnd ( /* ignore or code */ if ( STATE(IntrReturning) > 0 ) { return; } if ( STATE(IntrIgnoring) > 1 ) { STATE(IntrIgnoring)--; return; } - if ( STATE(IntrCoding) > 0 ) { CodeIfEnd( nr ); return; } - - /* if one branch was executed (ignoring the others) */ + // if one branch was executed (ignoring the others), reset IntrIgnoring if ( STATE(IntrIgnoring) == 1 ) { STATE(IntrIgnoring) = 0; - PushVoidObj(); } - /* if no branch was executed */ - else { - PushVoidObj(); - } + if ( STATE(IntrCoding) > 0 ) { CodeIfEnd( nr ); return; } + + PushVoidObj(); } diff --git a/src/intrprtr.h b/src/intrprtr.h index cfd120753e..9749265e37 100644 --- a/src/intrprtr.h +++ b/src/intrprtr.h @@ -204,7 +204,7 @@ extern void IntrIfElse ( void ); extern void IntrIfBeginBody ( void ); -extern void IntrIfEndBody ( +extern Int IntrIfEndBody ( UInt nr ); extern void IntrIfEnd ( diff --git a/src/read.c b/src/read.c index d6dcbcc9cc..485dc5f1a7 100644 --- a/src/read.c +++ b/src/read.c @@ -2124,8 +2124,7 @@ void ReadIf ( Match( S_THEN, "then", STATBEGIN|S_ELIF|S_ELSE|S_FI|follow ); TRY_READ { IntrIfBeginBody(); } nrs = ReadStats( S_ELIF|S_ELSE|S_FI|follow ); - TRY_READ { IntrIfEndBody( nrs ); } - nrb++; + TRY_READ { nrb += IntrIfEndBody( nrs ); } /* { 'elif' 'then' } */ while ( STATE(Symbol) == S_ELIF ) { @@ -2135,8 +2134,7 @@ void ReadIf ( Match( S_THEN, "then", STATBEGIN|S_ELIF|S_ELSE|S_FI|follow ); TRY_READ { IntrIfBeginBody(); } nrs = ReadStats( S_ELIF|S_ELSE|S_FI|follow ); - TRY_READ { IntrIfEndBody( nrs ); } - nrb++; + TRY_READ { nrb += IntrIfEndBody( nrs ); } } /* [ 'else' ] */ @@ -2145,8 +2143,7 @@ void ReadIf ( Match( S_ELSE, "else", follow ); TRY_READ { IntrIfBeginBody(); } nrs = ReadStats( S_FI|follow ); - TRY_READ { IntrIfEndBody( nrs ); } - nrb++; + TRY_READ { nrb += IntrIfEndBody( nrs ); } } /* 'fi' */ diff --git a/tst/testbugfix/2017-09-07-elseif.tst b/tst/testbugfix/2017-09-07-elseif.tst deleted file mode 100644 index 6ecada0d51..0000000000 --- a/tst/testbugfix/2017-09-07-elseif.tst +++ /dev/null @@ -1,24 +0,0 @@ -gap> f := function() if true then return 1; elif true then return 2; else return 3; fi; end;; -gap> Print(f, "\n"); -function ( ) - if true then - return 1; - elif true then - return 2; - else - return 3; - fi; - return; -end -gap> f := function() if false then return 4; elif false then return 5; else return 6; fi; end;; -gap> Print(f, "\n"); -function ( ) - if false then - return 4; - elif false then - return 5; - else - return 6; - fi; - return; -end diff --git a/tst/testinstall/coding.tst b/tst/testinstall/coding.tst new file mode 100644 index 0000000000..108c87acd7 --- /dev/null +++ b/tst/testinstall/coding.tst @@ -0,0 +1,133 @@ +# +# Test the GAP function coder +# +gap> START_TEST("coding.tst"); + +# +# Test coding of if-statements in which some conditions are true or false +# +# + +# test if-statements with single branch +gap> f:=function(x) if x then return 1; fi; end;; Display(f); +function ( x ) + if x then + return 1; + fi; + return; +end +gap> f:=function(x) if true then return 1; fi; end;; Display(f); +function ( x ) + return 1; +end +gap> f:=function(x) if false then return 1; fi; end;; Display(f); +function ( x ) + ; + return; +end + +# test if-statements with two branches (note that 'else' is equivalent to 'elif true'), +# first condition is neither true nor false +gap> f:=function(x) if x then return 1; else return 2; fi; end;; Display(f); +function ( x ) + if x then + return 1; + else + return 2; + fi; + return; +end +gap> f:=function(x) if x then return 1; elif x then return 2; fi; end;; Display(f); +function ( x ) + if x then + return 1; + elif x then + return 2; + fi; + return; +end +gap> f:=function(x) if x then return 1; elif false then return 2; fi; end;; Display(f); +function ( x ) + if x then + return 1; + fi; + return; +end + +# test if-statements with two branches (note that 'else' is equivalent to 'elif true'), +# first condition is true +gap> f:=function(x) if true then return 1; else return 2; fi; end;; Display(f); +function ( x ) + return 1; +end +gap> f:=function(x) if true then return 1; elif x then return 2; fi; end;; Display(f); +function ( x ) + return 1; +end +gap> f:=function(x) if true then return 1; elif false then return 2; fi; end;; Display(f); +function ( x ) + return 1; +end + +# test if-statements with two branches (note that 'else' is equivalent to 'elif true'), +# first condition is false +gap> f:=function(x) if false then return 1; else return 2; fi; end;; Display(f); +function ( x ) + return 2; +end +gap> f:=function(x) if false then return 1; elif x then return 2; fi; end;; Display(f); +function ( x ) + if x then + return 2; + fi; + return; +end +gap> f:=function(x) if false then return 1; elif false then return 2; fi; end;; Display(f); +function ( x ) + ; + return; +end + +# test some if-statements with three branches +gap> f:=function(x) if true then return 1; elif true then return 2; else return 3; fi; end;; Display(f); +function ( x ) + return 1; +end +gap> f:=function(x) if true then return 1; elif x then return 2; else return 3; fi; end;; Display(f); +function ( x ) + return 1; +end +gap> f:=function(x) if x then return 1; elif true then return 2; else return 3; fi; end;; Display(f); +function ( x ) + if x then + return 1; + else + return 2; + fi; + return; +end +gap> f:=function(x) if x then return 1; elif false then return 2; else return 3; fi; end;; Display(f); +function ( x ) + if x then + return 1; + else + return 3; + fi; + return; +end +gap> f:=function(x) if false then return 1; elif true then return 2; else return 3; fi; end;; Display(f); +function ( x ) + return 2; +end +gap> f:=function(x) if false then return 1; elif x then return 2; else return 3; fi; end;; Display(f); +function ( x ) + if x then + return 2; + else + return 3; + fi; + return; +end + +# +gap> STOP_TEST("coding.tst", 1); diff --git a/tst/testinstall/constant.tst b/tst/testinstall/constant.tst index 9d4a585856..73b1bdeffd 100644 --- a/tst/testinstall/constant.tst +++ b/tst/testinstall/constant.tst @@ -141,21 +141,8 @@ function ( ) return 3; return 5; ; - if true then - return 7; - elif 1 = 2 then - return 8; - else - return 9; - fi; - if false then - return 10; - elif true then - return 11; - else - return 12; - fi; - return; + return 7; + return 11; end gap> (function() if booltruevar then return 1; fi; return 2; end)(); 1