Add Constant Var support #1682

ChrisJefferson · 2017-09-06T15:15:11Z

This adds to gap the concept of a constant variable. They are created with MakeConstantVar, and behave similarly to MakeReadOnlyVar, except:

they can never be changed
they can only be small integers, true or false
The interpreter will inline them to optimise byte code.

The inlining is always done, and further to interpreter will inline away if statements of the form if constvar then... for a constant variable constvar.

fingolfin · 2017-09-06T16:03:09Z

doc/ref/language.xml

@@ -541,7 +541,30 @@ local variables or function arguments).
 <P/>
 First, such variables may be marked <E>read-only</E>. In which case
 attempts to change them will fail. Most of the global variables
-defined in the &GAP; library are so marked.
+defined in the &GAP; library are so marked. <E>read-only</E> variables
+can be made read-write again. GAP also features <E>constant</E> variables,


Perhaps: "can be made read-write again [using the command XYZ]"

fingolfin · 2017-09-06T16:05:13Z

lib/global.g

+##  named by the string <name> is constant and false otherwise (the default)
+##
+
+IS_CONSTANT_GLOBAL := IsConstantGVar;


Meta-note: It's really crazy that we define kernel function using CamelCase, then define alternate names in SNAKE_CASE, and finally high level names again in CamelCase. (Of course this PR is just imitating what the surrounding code does, so the PR is fine; I just think we may want to resolve this in another PR in the future)

fingolfin · 2017-09-06T16:08:16Z

src/code.c

+    // Some optimisation cases
+    if (nr == 1) {
+        if (TNUM_EXPR(cond1) == T_TRUE_EXPR) {
+            // Leave Statement


Statement -> statement

fingolfin · 2017-09-06T16:12:46Z

src/intrprtr.c

+**  'IntrSmallGAPIntExpr' is the action to 'interpret' a existing GAP small
+**  integer. This is used for implementing constants.
+*/
+void IntrSmallGAPIntExpr(Obj val)


I really prefer to refer to "immediate integers", as "small integer" is quite ambiguous (a reader may not realize it has a technical meaning). So perhaps IntrImmediateIntExpr and CodeImmediateIntExpr?

Or ...IntObj?

fingolfin · 2017-09-06T16:13:43Z

src/read.c

+            else if (IS_INTOBJ(val))
+                IntrSmallGAPIntExpr(val);
+            else
+                SyntaxError("Invalid Constant Variable");


"Invalid Constant Variable" -> "Invalid constant variable"

fingolfin · 2017-09-06T16:14:48Z

tst/testinstall/constant.tst

+    if boolfalsevar  then
+        return 6;
+    fi;
+    return;


Hmm, perhaps also test if/elif/fi and if/elif/else/fi ?

codecov · 2017-09-07T09:27:20Z

Codecov Report

Merging #1682 into master will increase coverage by <.01%.
The diff coverage is 83.16%.

@@            Coverage Diff             @@
##           master    #1682      +/-   ##
==========================================
+ Coverage   64.06%   64.07%   +<.01%     
==========================================
  Files         999      999              
  Lines      322458   322550      +92     
  Branches    13025    13064      +39     
==========================================
+ Hits       206593   206675      +82     
- Misses     113146   113151       +5     
- Partials     2719     2724       +5

Impacted Files	Coverage Δ
lib/global.g	`88.88% <ø> (ø)`	⬆️
src/code.h	`100% <ø> (ø)`	⬆️
src/gap.c	`56.26% <100%> (+0.14%)`	⬆️
src/intrprtr.c	`70.5% <66.66%> (-0.03%)`	⬇️
src/gvars.c	`77.79% <71.42%> (-0.01%)`	⬇️
src/stats.c	`71.95% <75%> (+0.2%)`	⬆️
lib/global.gi	`40.57% <83.33%> (+4.86%)`	⬆️
src/read.c	`81.59% <90%> (+0.06%)`	⬆️
src/code.c	`86.96% <93.93%> (+0.14%)`	⬆️
src/hpc/traverse.c	`77.9% <0%> (-0.31%)`	⬇️
... and 8 more

ChrisJefferson · 2017-09-07T09:29:29Z

I believe I have fixed issues, and written more tests. This PR includes a commit which fixes printing elif true, which was always printed as else.

I agree the camel case / allcaps dance is weird (and I would like to remove the methods MakeConstantGVar and friends, as to a user I think it's unclear if they should be using MakeConstantGVar and MakeConstantGlobal).

ChrisJefferson · 2017-09-07T09:32:22Z

Note the optimisations this supports are currently very limited. It only simplifies code of the form if true then or if false then. We could easily support more advanced simplifications, but I would prefer to add only a small set (a future system based on turning bytecode into GAP objects could do clever, more extensive, rewrites).

fingolfin · 2017-09-09T23:04:19Z

BTW, why do you capitalise "Constant" in e.g. the commit message "Add Constant support" ? Shouldn't it be rather "Add support for constant variables" or so?

fingolfin · 2017-09-09T23:06:28Z

lib/global.g

@@ -92,6 +102,16 @@ MAKE_READ_WRITE_GLOBAL := MakeReadWriteGVar;

 #############################################################################
 ##
+#F  MAKE_CONSTANT_GLOBAL ( <name> ) . . .make a global variable constant


add at least a space before "make". But actually there should be enough . . . to right-align the text afterwards... Alternatively, just remove the whole . . .make a global variable constant bit, as it is kind of redundant given the following text.

Still would like to see that space (or, as in my other comment: Remove that line).

fingolfin · 2017-09-09T23:07:55Z

lib/global.gd

@@ -155,6 +170,26 @@ DeclareGlobalFunction("MakeReadWriteGlobal");

 #############################################################################
 ##
+#F  MakeConstantGlobal( <name> )   . . . . make a global variable constant


again the . . . business... should change the triple spaces to a single one and insert more . -- the same in other places, too, of course

I copied the triple spaces from the function directly above, and below, this one. This file has either triple spaces, or double spaces, never a single one.

There are 25 places in the whole library using a triple space in that location, i.e. less than 1% -- I think they come from somebody not being careful and/or not caring.

Now, I am first to agree that it sucks to manually update these kinds of comments. But I still think there are only two sane choices:

Abolish them, possibly incrementally (by just not adding them to new code)

Keep using them "as intended" -- and then triple spaces to me look quite clearly like a bug, which simply proliferates by copy&paste.

tl;dr: please either delete this line, or format it to match the rest of the library (and not just the rest of this comparatively short file).

fingolfin · 2017-09-09T23:08:48Z

lib/global.gd

+##  MakeConstantGlobal ( <A>name</A> ) marks the global variable named
+##  by the string <A>name</A> as constant. A constant variable can never
+##  be changed or made read-write. Constant variables can only take an
+##  integer value, <C>true</C> or <C>false</C>.


should that be "immediate integer value"? Hmm, though not everybody will know what that means..

I don't think the concept is defined in the documentation -- not anywhere obvious, anyway. Maybe just say that there is a limit on the size of the integers.

fingolfin · 2017-09-09T23:09:52Z

lib/global.gi

+##  MakeConstantGlobal ( <name> ) marks the global variable named
+##  by the string <name> as constant
+##
+## A warning is given if <name> is already constant


Add extra space before A warning

fingolfin · 2017-09-09T23:10:02Z

lib/global.gi

+##
+#F  MakeConstantGlobal ( <name> )  . . . . make a global variable constant
+##
+##  MakeConstantGlobal ( <name> ) marks the global variable named


Why the weird wrapping here?

once again, same as the wrapping on the function just before

fingolfin · 2017-09-09T23:12:28Z

doc/ref/language.xml

+can be made read-write again by calling <Ref Func="MakeReadWriteGlobal"/>.
+GAP also features <E>constant</E> variables,
+which can never be changed. In some cases, GAP can optimise code which
+uses <E>constant</E> variables, as their value never changes.


Hmm, the user at this point knows very little about constants, and has no idea how to learn more. E.g. they won't know that only immediate integers and some booleans can be made constant. I'd add a ref to MakeConstantGVar here, and then make sure they can learn enough about that in its documentation (which currently does not seem to exist...?).

Getting back to this, it seems this this should be MakeConstantGlobal instead (also in the example below)

fingolfin · 2017-09-09T23:29:49Z

src/code.c

+        Stat body2 = PopStat();
+        cond2 = PopExpr();
+        PushExpr(cond2);
+        PushStat(body2);


You don't really need to pop/push body2 (and the original code also did not need to push/pop body...

fingolfin · 2017-09-09T23:31:38Z

src/code.c

+            // Remove entire if statement
+            PopStat();
+            PopExpr();
+            PushStat(NewStat(T_EMPTY, 0));


Hmm... is that PushStat really necessary?

fingolfin · 2017-09-09T23:38:48Z

src/gvars.c

@@ -799,10 +856,9 @@ Obj FuncMakeReadWriteGVar (
 Int IsReadOnlyGVar (
    UInt                gvar )
 {
-    return !INT_INTOBJ(ELM_GVAR_LIST(WriteGVars, gvar));
+    return INT_INTOBJ(ELM_GVAR_LIST(WriteGVars, gvar)) == 0;


Rewrite this as return ELM_GVAR_LIST(WriteGVars, gvar) == INTOBJ_INT(0)

fingolfin · 2017-09-09T23:40:08Z

src/gvars.c

+    Obj                 self,
+    Obj                 name )
+{
+    /* check the argument                                                  */


Personally, I try to avoid those "right aligned with lots of spaces" comments, and just use C++ style // comments. While this makes the new code slightly different than the old code, I think we should eventually replace all those weird aligned comments with these

(That's just a vague suggestion, not an actual request to change the PR).

fingolfin · 2017-09-17T21:52:42Z

lib/global.gi

+        function (name)
+    local isro;
+    CheckGlobalName( name );
+    isro := IS_CONSTANT_GLOBAL(name);


Why "isro" ? Probably copy & paste from IsReadOnlyGlobal, but this name doesn't make sense here.

fingolfin · 2017-09-17T21:55:52Z

src/code.c

@@ -1616,6 +1654,11 @@ void CodePow ( void )
 }


+void CodeGAPSmallInt(Obj val)


I'd really prefer if this was called CodeImmediateInt -- saying "small int" is quite ambiguous. This code instead deals with "immediate integer handles" (to use the language from gmpints.c)

[ Sorry, I thought I had mentioned this in my previous PR? But perhaps I failed to actually submit that comment? I can't find it now, in any case :-/ ]

fingolfin · 2017-09-17T21:57:52Z

src/gap.c

 #ifdef HPCGAP
    UInt var = GVarName( "HPCGAP" );
    AssGVar( var, True );
-    MakeReadOnlyGVar( var );
+    MakeConstantGVar( var );


Perhaps IsBound(CONST_VAR) should be optimized true if CONST_VAR is bound during coding? (I am not asking for this to be done in this PR, just wondering)

fingolfin · 2017-09-17T21:59:29Z

src/gvars.c

+*/
+Int IsConstantGVar(UInt gvar)
+{
+    return INT_INTOBJ(ELM_GVAR_LIST(WriteGVars, gvar)) == -1;


Better (and slightly safer...) code can be generated if you instead write return ELM_GVAR_LIST(WriteGVars, gvar) == INTOBJ_INT(-1); -- this would then also match IsReadOnlyGVar above.

fingolfin · 2017-09-17T22:04:54Z

lib/global.gd

@@ -155,6 +170,26 @@ DeclareGlobalFunction("MakeReadWriteGlobal");

 #############################################################################
 ##
+#F  MakeConstantGlobal( <name> )   . . . . make a global variable constant


There are 25 places in the whole library using a triple space in that location, i.e. less than 1% -- I think they come from somebody not being careful and/or not caring.

Now, I am first to agree that it sucks to manually update these kinds of comments. But I still think there are only two sane choices:

Abolish them, possibly incrementally (by just not adding them to new code)

Keep using them "as intended" -- and then triple spaces to me look quite clearly like a bug, which simply proliferates by copy&paste.

tl;dr: please either delete this line, or format it to match the rest of the library (and not just the rest of this comparatively short file).

fingolfin · 2017-09-17T22:15:31Z

src/gvars.c

@@ -825,6 +881,34 @@ static Obj FuncIsReadOnlyGVar (
    return IsReadOnlyGVar(GVarName(CSTR_STRING(name))) ? True : False;
 }

+/****************************************************************************
+**
+*F  IsConstantGVar( <gvar> ) . . . . . . return if a variable is a constant


Hrmf, somehow GitHub decided that the comments I was collecting for a review were all to be suddenly published as comments instead, taking away from me the possibility to edit them beforehand... I was about to add the following (or possibly, replace those comments completely with it) regarding the comment formatting: "OK, I see now that these files you are touching are particularly bad when it comes to the comment formatting (and grammar...). So it probably makes more sense to go over them separately later on, and to "clean up" those comments, and/or to remove parts of them."

fingolfin

Overall, this looks good to me now. There are some minor formatting issues in my view (such as places with double empty lines or trailing spaces, new code which has comments of the form /* some text + a zillion spaces for right-aligning */ instead of // some text, etc., and perhaps 1-2 other minor chances for improvements, but nothing really that warrants holding this back much longer.

I'd still feel better if @stevelinton or @markuspf also had a look at this before merging.

stevelinton · 2017-09-19T21:48:25Z

My only concern with this is that if we want to take it further and have a "proper" optimisation framework then quite a bit of it will be superceded. On reflection though, it would be the implementation, and maybe the printing of functions that had optimised versions that would change, and not the UI. This may be developed further in future -- for instance a Constant Var holding a function would allow inlining. I imagine in future we might also stop printing the optimised versions of functions, but rather hold the original and the optimised bodys and print one while running the other, so again maybe the documentation should make clear that this may change, but we promise it will never hurt to make constant things constant.

ChrisJefferson · 2017-09-19T22:02:28Z

I agree about saying that we don't promise to show optimised functions.

I would prefer a general framework based on @markuspf's transforming of GAP functions to GAP objects -- in particular I would love GAP to have inlining.

Also, I don't think we should expand this much further -- it would be easy to continue through the parser and handle things like true or X is true, but then we would end up with effectively "3" execution paths -- immediate execution, turning into bytecode and execution while generating bytecode.

fingolfin · 2017-09-19T23:22:02Z

Supporting simplification of true or X, IsBound(constVar), false and X, etc. actually seems pretty much straightforward, and I wouldn't consider it a "third execution path" -- OK, in some sens it is, but this is really basic stuff, running in guaranteed constant time. On the plus side, it would make it possible to simply expressions involving multiple constant vars.

Of course it's tricky were to draw the line... Simplifying expressions like 1+1, or, more realistically, 2^20, would be a natural next step, and again pretty easy to implement, but of course now things get way more complex, as the result may not fit into an immediate integer, or may even raise an error (think 1/0).

And I really think this would already provide some nice (minor) speed up for some code, without needing the full power (and considerable complexity) of a full code transformation framework...

Anyway, that's just some thoughts, but of course they in no way speak against this PR :-).

stevelinton · 2017-09-21T21:57:26Z

doc/ref/language.xml

+GAP also features <E>constant</E> variables, which are created by calling
+<Ref Func="MakeConstantGlobal"/>. Constant variables can never be changed.
+In some cases, GAP can optimise code which uses <E>constant</E> variables,
+as their value never changes. This can be observed by printing the


I would delete the sentence beginning "This can be observed", or add a warning that that might change.
Otherwise approved.

stevelinton · 2017-09-21T22:04:17Z

lib/global.gd

+##  MakeConstantGlobal ( <A>name</A> ) marks the global variable named
+##  by the string <A>name</A> as constant. A constant variable can never
+##  be changed or made read-write. Constant variables can only take an
+##  integer value, <C>true</C> or <C>false</C>.


I don't think the concept is defined in the documentation -- not anywhere obvious, anyway. Maybe just say that there is a limit on the size of the integers.

olexandr-konovalov · 2017-10-21T09:41:47Z

@ChrisJefferson just adding ManSection to the documentation does not automatically add it to the manual - it needs to be hooked somewhere via the Include mechanism. This is why cross-reference to MakeConstantGlobal is not resolved, as you can see at https://travis-ci.org/gap-system/gap/jobs/279972109. Unfortunately, that test does not fail automatically when cross-references are not resolved (it only fails if the build completely fails to produce manuals), so one should check it manually for any PR that updates manuals.

Perhaps we can detect unresolved cross-references too - but should carefully do that to ignore reports on them during the first run of the build.

fingolfin reviewed Sep 6, 2017

View reviewed changes

ChrisJefferson force-pushed the constant branch 2 times, most recently from 82d442d to 76d68de Compare September 7, 2017 09:26

ChrisJefferson force-pushed the constant branch from 76d68de to 838d42c Compare September 7, 2017 11:02

olexandr-konovalov added the gapdays2017-fall Issues and PRs that arose at https://www.gapdays.de/gapdays2017-fall label Sep 9, 2017

fingolfin requested changes Sep 9, 2017

View reviewed changes

ChrisJefferson force-pushed the constant branch 3 times, most recently from ab2a9dd to f245a79 Compare September 16, 2017 19:44

fingolfin reviewed Sep 17, 2017

View reviewed changes

fingolfin approved these changes Sep 17, 2017

View reviewed changes

fingolfin requested review from markuspf and stevelinton September 19, 2017 16:45

fingolfin added kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements kind: new feature labels Sep 19, 2017

fingolfin added this to the GAP 4.9.0 milestone Sep 19, 2017

stevelinton approved these changes Sep 21, 2017

View reviewed changes

ChrisJefferson added 3 commits September 26, 2017 14:46

Fix printing of 'elif true'

ebea5b9

Add constant support

219daf7

Add IsHPCGAP global Constant variable

96f3d2a

ChrisJefferson force-pushed the constant branch from f245a79 to 96f3d2a Compare September 26, 2017 13:52

markuspf merged commit 221df9d into gap-system:master Oct 2, 2017

ChrisJefferson deleted the constant branch November 6, 2017 12:44

fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Dec 13, 2017

		@@ -1616,6 +1654,11 @@ void CodePow ( void )
		}


		void CodeGAPSmallInt(Obj val)

Add Constant Var support #1682

Add Constant Var support #1682

Conversation

ChrisJefferson commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 7, 2017 • edited Loading

Codecov Report

ChrisJefferson commented Sep 7, 2017

ChrisJefferson commented Sep 7, 2017

fingolfin commented Sep 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin left a comment

Choose a reason for hiding this comment

stevelinton commented Sep 19, 2017

ChrisJefferson commented Sep 19, 2017

fingolfin commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olexandr-konovalov commented Oct 21, 2017

codecov bot commented Sep 7, 2017 •

edited

Loading