Skip to content

Commit

Permalink
Optimize true/false conditions when coding if-statements
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
fingolfin committed Feb 22, 2018
1 parent a218d68 commit eba3edb
Show file tree
Hide file tree
Showing 8 changed files with 194 additions and 98 deletions.
12 changes: 0 additions & 12 deletions lib/system.g
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
81 changes: 37 additions & 44 deletions src/code.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand All @@ -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 */
Expand Down
2 changes: 1 addition & 1 deletion src/code.h
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ extern void CodeIfElse ( void );

extern void CodeIfBeginBody ( void );

extern void CodeIfEndBody (
extern Int CodeIfEndBody (
UInt nr );

extern void CodeIfEnd (
Expand Down
36 changes: 17 additions & 19 deletions src/intrprtr.c
Original file line number Diff line number Diff line change
Expand Up @@ -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; }

Expand Down Expand Up @@ -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-- ) {
Expand All @@ -621,6 +621,8 @@ void IntrIfEndBody (

/* one branch of the if-statement was executed, ignore the others */
STATE(IntrIgnoring) = 1;

return 1;
}

void IntrIfEnd (
Expand All @@ -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();
}


Expand Down
2 changes: 1 addition & 1 deletion src/intrprtr.h
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ extern void IntrIfElse ( void );

extern void IntrIfBeginBody ( void );

extern void IntrIfEndBody (
extern Int IntrIfEndBody (
UInt nr );

extern void IntrIfEnd (
Expand Down
9 changes: 3 additions & 6 deletions src/read.c
Original file line number Diff line number Diff line change
Expand Up @@ -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' <Expr> 'then' <Statments> } */
while ( STATE(Symbol) == S_ELIF ) {
Expand All @@ -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' <Statments> ] */
Expand All @@ -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' */
Expand Down
133 changes: 133 additions & 0 deletions tst/testinstall/coding.tst
Original file line number Diff line number Diff line change
@@ -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);
Loading

0 comments on commit eba3edb

Please sign in to comment.