Compile functions to a GAP record #1132

markuspf · 2017-02-09T21:28:21Z

This PR adds a function SYNTAX_TREE which takes as its argument a GAP function and returns a record that describes that function's syntax tree. This will be useful for a range of purposes such as code analysis, constant folding, optimisations, testing, which can be done in GAP instead of awkwardly coding it in C.

The code is somewhat mirroring the Compiler/Executor/Coder (All three already look very similar), but could be simplified quite a bit, because bar some more complex constructs, most statements/expressions are binary or ternary operations and nothing else.

It is still lacking tests and documentation, and probably has quite a few bugs left, but I'll open this PR for people to see what's going on.

It also does not support HPC-GAP syntax at all at this point.

It would probably be quite nice to have an "inverse" of this function that takes a syntax tree and compiles it back to GAP bytecode, as well as exploring the option of hooking into the parser directly to produce the syntax tree instead of interpreting bytecode.

ChrisJefferson

some minor early comments

ChrisJefferson · 2017-02-09T21:32:31Z

src/syntaxtree.c

+    return result;
+}
+
+/* TODO: Can we still identify whether a string was triple quoted? */


No, internally the strings are stored identically

ChrisJefferson · 2017-02-09T21:33:26Z

src/syntaxtree.c

+    result = NewSyntaxTreeNode("And",3);
+
+    AssPRec(result, RNamName("left"), SyntaxTreeExpr(ADDR_EXPR(expr)[0]));
+    AssPRec(result, RNamName("right"), SyntaxTreeExpr(ADDR_EXPR(expr)[0]));


[1]?

In general I might be tempted to make this into some kind of macro, just because we have lots of 2-op and 1-op cases to handle.

yes, I was planning that anyway (there must be a comment around somewhere that says that these functions are all the same and they should be a macro or inline function)

ChrisJefferson · 2017-02-09T21:34:38Z

src/syntaxtree.c

+    return result;
+}
+
+Obj SyntaxTreePermExpr (Expr expr)


Would it be easier / simpler to just give a GAP permutation?

well, since you can have expressions in the permutation entries, I want to be able to reflect that, i.e. what do you do with

for i in [1..5] do p := p * (i, i+5); od;

Ah yes, I keep forgetting you can do that :)

codecov-io · 2017-02-20T02:06:14Z

Codecov Report

Merging #1132 into master will decrease coverage by 33.1%.
The diff coverage is 3.49%.

@@             Coverage Diff             @@
##           master    #1132       +/-   ##
===========================================
- Coverage   60.92%   27.82%   -33.11%     
===========================================
  Files         997       92      -905     
  Lines      301950    55297   -246653     
  Branches    13036    10750     -2286     
===========================================
- Hits       183968    15385   -168583     
+ Misses     115333    38380    -76953     
+ Partials     2649     1532     -1117

Impacted Files	Coverage Δ
src/gap.c	`44.53% <ø> (-11.74%)`	⬇️
src/syntaxtree.c	`3.49% <3.49%> (ø)`
src/fibhash.h	`0% <0%> (-100%)`	⬇️
src/objfgelm.h	`0% <0%> (-100%)`	⬇️
src/pperm.h	`0% <0%> (-100%)`	⬇️
src/trans.c	`3.16% <0%> (-95.73%)`	⬇️
src/tietze.c	`1.2% <0%> (-82.78%)`	⬇️
src/pperm.c	`2.22% <0%> (-75.92%)`	⬇️
src/objset.c	`9.62% <0%> (-72.98%)`	⬇️
src/vector.c	`19.9% <0%> (-72.26%)`	⬇️
... and 987 more

fingolfin · 2017-02-20T15:23:31Z

src/syntaxtree.c

+
+Obj SyntaxTreeUnknownBool(Expr expr)
+{
+    /* compile the expression and check that the value is boolean          */


Irrelevant nitpick: Could we perhaps agree to not space pad comments in new code? I think it's a horrible waste of time to go through functions to ensure comments are padded consistently; and if one doesn't do it, it looks just weird. Your TODO comments already follow this, after all (and I'd leave the #include comments alone for now; those are changed by a perl script in the hpc-merge branch anyway)

fingolfin · 2017-02-20T15:24:54Z

src/syntaxtree.c

+        siz = SIZE_EXPR(expr) - sizeof(UInt);
+        typ = *(UInt *)ADDR_EXPR(expr);
+        value = C_MAKE_INTEGER_BAG(siz, typ);
+        for ( i = 0; i < siz/INTEGER_UNIT_SIZE; i++ ) {


This for and the if a few lines above are inconsistenly formatted, if(FOO) vs. for ( FOO ). Perhaps use clang-format on the new code?

fingolfin · 2017-02-20T15:31:36Z

src/syntaxtree.c

+#endif
+        }
+        if (siz <= 8) {
+            value = C_NORMALIZE_64BIT(value);


Oops, I wasn't even aware of C_NORMALIZE_64BIT and the above code (which comes from the compiler, I think?). I'd prefer if all integer normalization was handled by code in gmpints.c, and if code outside assumed as little as possible about the internal format of the integer code, so that future changes require less code changes outside of gmpints.c. I'll try to understand this code and see if I can suggest an alternative...

fingolfin · 2017-02-20T15:35:04Z

src/syntaxtree.c

+
+    result = NewSyntaxTreeNode("IntExpr");;
+
+    if(IS_INTEXPR(expr)) {


Never noticed this before, but: T_INT_EXPR vs. T_INTEXPR seems needlessly confusing; perhaps we should rename one or both, to indicate one is immediate / small, the other big?

Also gotta love how EvalIntExpr of course deals with T_INT_EXPR but not T_INTEXPR ;-).

fingolfin · 2017-02-20T15:37:00Z

src/syntaxtree.c

+#elif INTEGER_UNIT_SIZE == 8
+            C_SET_LIMB8(value, i, ((UInt8 *)((UInt *)ADDR_EXPR(expr) + 1))[i]);
+#else
+            #error unsupported INTEGER_UNIT_SIZE


Hmm, I am not sure I understand why you need to use these compiler macros in order to convert the expression into an integer object. Can't you just do what EvalIntExpr does?

fingolfin · 2017-02-20T15:38:01Z

src/syntaxtree.c

+    return result;
+}
+
+Obj SyntaxTreeIntExpr(Expr expr)


This, and other functions like it, is never referenced anywhere. So I guess it's incomplete work in progress?

fingolfin · 2017-02-20T15:38:58Z

src/syntaxtree.h

+*/
+StructInitInfo * InitInfoSyntaxTree ( void );
+
+#endif // GAP_COMPILER_H


should be GAP_SYNTAXTREE_H not GAP_COMPILER_H

fingolfin · 2017-02-20T15:39:36Z

src/syntaxtree.c

+
+    { T_REFLVAR, SyntaxTreeDefaultExpr, "T_REFLVAR", 0, { } },
+
+    DEF_EXPR_DEFAULT(41),


This gives a compiler error for me:

../../src/syntaxtree.c:1344:5: error: initializer element is not a compile-time constant DEF_EXPR_DEFAULT(41), ^~~~~~~~~~~~~~~~~~~~

fingolfin · 2017-02-20T15:40:11Z

src/syntaxtree.c

+
+    /* compile the statements                                              */
+    for ( i = 1; i <= nr; i++ ) {
+        SET_ELM_PLIST(list, i, SyntaxTreeStat( ADDR_STAT( stat )[i-1] ) );


../../src/syntaxtree.c:553:32: warning: implicit declaration of function 'SyntaxTreeStat' is invalid in C99 [-Wimplicit-function-declaration] SET_ELM_PLIST(list, i, SyntaxTreeStat( ADDR_STAT( stat )[i-1] ) ); ^

... and lot of other warnings... WIP code?

fingolfin · 2017-03-09T12:29:05Z

src/syntaxtree.c

+}
+
+static ArgT[] = {
+    ARG_COMPILER("rnam", SyntaxTreeCompiler);


ARG_COMPILER doesn't seem to be defined anywhere? Maybe ARG_ is meant?

fingolfin · 2017-05-27T18:56:05Z

src/syntaxtree.c

+static const CompilerT ExprCompilers[];
+
+#define COMPILER_ARITY(...)                                                  \
+    (sizeof((ArgT []){ __VA_ARGS__ }) / sizeof(ArgT))


Unfortunately this macro (or rather, its expansion) trips up HPC-GAP.

No idea how hard it would be to teach ward about this C99 feature. But of course a simple fix would be to forgo variadics, and simply have several COMPILER macros: for 0, 1, 2, ... args. But that's certainly less elegant :-/

stevelinton · 2017-10-03T16:31:16Z

syntaxtree.gd seems to be missing

fingolfin · 2017-10-19T10:44:28Z

tst/testinstall/syntax/syntaxtree.tst

+gap> Read( Filename(DirectoriesLibrary( "tst/testinstall/syntax" ), "testsyntax.g" ) );
+gap> i := PositionsProperty([1..Length(trees)], i -> trees[i] <> expect_trees[i]);;
+gap> List(trees{ i }, x -> x.name);
+[  ]


This fails, actually gives [ "stat_test_if", "test_comobj" ]

The former failure is probably because we now optimize if true and if false away in certain cases.

markuspf · 2017-10-24T11:40:36Z

I don't see a point in leaving this open if work-in-progress leads to distraction.

markuspf added do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: enhancement Label for issues suggesting enhancements; and for pull requests implementing enhancements labels Feb 9, 2017

markuspf self-assigned this Feb 9, 2017

ChrisJefferson reviewed Feb 9, 2017

View reviewed changes

markuspf force-pushed the compile-to-gap-record branch from b2dceca to 94d0dd0 Compare February 20, 2017 12:22

fingolfin reviewed Feb 20, 2017

View reviewed changes

markuspf force-pushed the compile-to-gap-record branch from 1710701 to 8a1dcb4 Compare February 26, 2017 01:31

markuspf force-pushed the compile-to-gap-record branch from 706d81e to 9385490 Compare March 8, 2017 22:03

fingolfin reviewed Mar 9, 2017

View reviewed changes

markuspf force-pushed the compile-to-gap-record branch from 9385490 to 029a653 Compare May 27, 2017 16:14

fingolfin reviewed May 27, 2017

View reviewed changes

markuspf force-pushed the compile-to-gap-record branch from 7abb8a8 to 4ad3dba Compare September 26, 2017 14:49

markuspf added 17 commits October 2, 2017 14:54

Add compilation to syntax tree

bd3fa96

Some cleanup and error fixing

41a710f

First compiling and "working" version

e6a7df2

Fix a few crashes

60efc0f

Whitespace Cleanup

4f9ea87

Bug and Warning squashing

7b166dc

Make more operations work

b599db5

Character literals

0713525

PermExpr

e510c5e

Some cleanup

6593d54

Captialisation and further cleanup

6ac0340

Make more functions return sensible values and fix Tilde

e4687c6

More cleanup

43d6df1

Better handling of functions

6146ad2

Squashing some warnings (and bugs).

6eeaaee

Fix Typo/Bug

60b4e21

Fix bug with empty statement lists

32f975f

markuspf added 23 commits October 2, 2017 14:54

Remove LVAR stuff

b842e59

Fixes

6f98b28

Updates

4b2e928

Use CURR_FUNC()

c5547c3

Add some example compilers for syntax trees

f25f31a

Fix For-Loops and Rename "then" body.

c390168

Correct While and Repeat

eb3a0f6

Add some tests for SYNTAX_TREE

ca111e8

Bugfixes for LVar references

7d388c6

Rename "function" component to "funcref"

da8ec02

Add testcase for funccall/proccall

d06bbcc

Remove superflous example code

53bc746

Implement ELM2_LIST, ASS2_LIST, ELMX_LIST, ASSX_LIST

1b60432

Update testdata

b8b15b0

Sort statement types

fb94964

A cleanup compiler

157efbc

Updates

ab6fa33

Fix T_UNB_COMOBJ_NAME

9712f0a

Add SyntaxTree and Compiler Types. Refactor Compiler

7d9e83e

More work on compilers

d50bd8b

Testing updates

253dcea

Remove T_INV

15004a9

More work on the pretty printer

fe52f6f

markuspf force-pushed the compile-to-gap-record branch from 4ad3dba to fe52f6f Compare October 3, 2017 15:19

Add syntaxtree.gd

50c0a0d

markuspf mentioned this pull request Oct 9, 2017

Add functions to View, Display, and Code (formerly Print) to a stream #895

Closed

fingolfin reviewed Oct 19, 2017

View reviewed changes

markuspf closed this Oct 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile functions to a GAP record #1132

Compile functions to a GAP record #1132

markuspf commented Feb 9, 2017

ChrisJefferson left a comment

ChrisJefferson Feb 9, 2017

ChrisJefferson Feb 9, 2017

markuspf Feb 9, 2017

ChrisJefferson Feb 9, 2017

markuspf Feb 9, 2017

ChrisJefferson Feb 9, 2017

codecov-io commented Feb 20, 2017 •

edited by codecov bot

Loading

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Feb 20, 2017

fingolfin Mar 9, 2017

fingolfin May 27, 2017

stevelinton commented Oct 3, 2017

fingolfin Oct 19, 2017

fingolfin Oct 19, 2017

markuspf commented Oct 24, 2017


		result = NewSyntaxTreeNode("IntExpr");;

		if(IS_INTEXPR(expr)) {


		{ T_REFLVAR, SyntaxTreeDefaultExpr, "T_REFLVAR", 0, { } },

		DEF_EXPR_DEFAULT(41),

Compile functions to a GAP record #1132

Compile functions to a GAP record #1132

Conversation

markuspf commented Feb 9, 2017

ChrisJefferson left a 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

codecov-io commented Feb 20, 2017 • edited by codecov bot Loading

Codecov Report

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

stevelinton commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuspf commented Oct 24, 2017

codecov-io commented Feb 20, 2017 •

edited by codecov bot

Loading