Skip to content
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

Constant folding for Python and other runtimes #3701

Merged
merged 2 commits into from
Aug 31, 2022

Conversation

KvanTTT
Copy link
Member

@KvanTTT KvanTTT commented May 7, 2022

@SimonStPeter I've created a benchmark with respective grammar and have got the following result:

no_const_folding: 2010734 ns
const_folding: 907140 ns
ratio: 0.4511

I haven't tested on real grammars, but It's already signficant improvement, more 2x faster and it should be considered. Also, resulting file > 20 % less than the file with not folded constants.

There is no improvement in speed for JavaScript.

fixes #3698, fixes #3699

@parrt I suggest using constant folding for all runtimes because:

  • It reduces the size of generated parsers (more than 20% as I've checked for Python and JavaScript)
  • It eliminates very long lines that are hard to read by human and text editors. Just compare (there are a lot of similar lines for SQL parsers):
    • Current: if not(((((_la - 1)) & ~0x3f) == 0 and ((1 << (_la - 1)) & ((1 << (PParser.T0 - 1)) | (1 << (PParser.T1 - 1)) | (1 << (PParser.T2 - 1)) | (1 << (PParser.T3 - 1)) | (1 << (PParser.T4 - 1)) | (1 << (PParser.T5 - 1)) | (1 << (PParser.T6 - 1)) | (1 << (PParser.T7 - 1)) | (1 << (PParser.T8 - 1)) | (1 << (PParser.T9 - 1)) | (1 << (PParser.T10 - 1)) | (1 << (PParser.T11 - 1)) | (1 << (PParser.T12 - 1)) | (1 << (PParser.T13 - 1)) | (1 << (PParser.T14 - 1)) | (1 << (PParser.T15 - 1)) | (1 << (PParser.T16 - 1)) | (1 << (PParser.T17 - 1)) | (1 << (PParser.T18 - 1)) | (1 << (PParser.T19 - 1)) | (1 << (PParser.T20 - 1)) | (1 << (PParser.T21 - 1)) | (1 << (PParser.T22 - 1)) | (1 << (PParser.T23 - 1)) | (1 << (PParser.T24 - 1)) | (1 << (PParser.T25 - 1)) | (1 << (PParser.T26 - 1)) | (1 << (PParser.T27 - 1)) | (1 << (PParser.T28 - 1)) | (1 << (PParser.T29 - 1)) | (1 << (PParser.T30 - 1)) | (1 << (PParser.T31 - 1)) | (1 << (PParser.T32 - 1)) | (1 << (PParser.T33 - 1)) | (1 << (PParser.T34 - 1)) | (1 << (PParser.T35 - 1)) | (1 << (PParser.T36 - 1)) | (1 << (PParser.T37 - 1)) | (1 << (PParser.T38 - 1)) | (1 << (PParser.T39 - 1)) | (1 << (PParser.T40 - 1)) | (1 << (PParser.T41 - 1)) | (1 << (PParser.T42 - 1)) | (1 << (PParser.T43 - 1)) | (1 << (PParser.T44 - 1)) | (1 << (PParser.T45 - 1)) | (1 << (PParser.T46 - 1)) | (1 << (PParser.T47 - 1)) | (1 << (PParser.T48 - 1)) | (1 << (PParser.T49 - 1)) | (1 << (PParser.T50 - 1)) | (1 << (PParser.T51 - 1)) | (1 << (PParser.T52 - 1)) | (1 << (PParser.T53 - 1)) | (1 << (PParser.T54 - 1)) | (1 << (PParser.T55 - 1)) | (1 << (PParser.T56 - 1)) | (1 << (PParser.T57 - 1)) | (1 << (PParser.T58 - 1)) | (1 << (PParser.T59 - 1)) | (1 << (PParser.T60 - 1)) | (1 << (PParser.T61 - 1)) | (1 << (PParser.T62 - 1)) | (1 << (PParser.T63 - 1)))) != 0)):
    • Folded: if not((((_la - 1) & ~0x3f) == 0 and (1 << (_la - 1)) & -1) != 0):
  • Actually such expressions almost don't clarity how parser works in debug mode (if it's important).
  • It improves compilation speed (for compilable languages) or interpretation performance (for dynamic languages such as Python or JavaScript)

@KvanTTT
Copy link
Member Author

KvanTTT commented May 7, 2022

CI improvement for Python: 12s vs >13s vs (>10%)

@parrt
Copy link
Member

parrt commented May 7, 2022

Looks cool. I'll check as soon as I can.

@SimonStPeter
Copy link

SimonStPeter commented May 7, 2022 via email

@KvanTTT KvanTTT force-pushed the constant-folding branch 2 times, most recently from 2e2ccb5 to 0b22d5a Compare May 8, 2022 15:21
@KvanTTT
Copy link
Member Author

KvanTTT commented May 8, 2022

Also I've completed the following:

  • Go: getInlineTestSetWordSize 32 -> 64 (go actually supports long integers)
  • Dart: get rid of BigInt
  • Swift: optimize TestSetInline (also should improve performance)

@KvanTTT KvanTTT changed the title Constant folding for Python and JavaScript runtimes Constant folding for Python and other runtimes May 8, 2022
@parrt
Copy link
Member

parrt commented Jun 25, 2022

Should we look at this again after completing your other testing upgrades? @KvanTTT

@KvanTTT
Copy link
Member Author

KvanTTT commented Jun 25, 2022

Yes, I think it makes sense to get rid of very long lines in generated sources.

@KvanTTT
Copy link
Member Author

KvanTTT commented Jul 3, 2022

Eventually, it's ready for review.

@KvanTTT
Copy link
Member Author

KvanTTT commented Aug 23, 2022

@parrt any chance it will be merged?

@parrt parrt mentioned this pull request Aug 27, 2022
10 tasks
@parrt
Copy link
Member

parrt commented Aug 27, 2022

Happy to merge once you add the parentheses and change the name of the method! thanks!

@@ -87,4 +87,7 @@ public boolean wantsBaseVisitor() {
public boolean supportsOverloadedMethods() {
return false;
}

@Override
public boolean supportsConstants() { return false; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hang on. I'm just not getting this. Why not just always do this if it's better? All targets can handle bitsets right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, this is turned off for Python but Python is what we want to fix. Maybe flip the boolean sense of the function name so True means do this new stuff.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I asked this via email (which got lost), so again:

Hi, silly question - why conditionally fold, why not just always fold for all targets? Seems odd, am I misunderstanding something?

(worth having a 'do not fold' option for debugging purposes, so you can examine the output, but only for human use).

cheers

jan

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Further, as I can't find the right post and my other email also disappeared, there's a question about the nomenclature of 'folding' and what a 'constant' is:

Ivan says > "is being folded to constant 0xFFFF. But Python doesn't support constant propagation since Python is unable to determine whether a variable is constant or not."

My take is folding = evaluation and a constant here is more accurately described as a literal.

I love the smell of bikeshedding in the morning...

Copy link
Member Author

@KvanTTT KvanTTT Aug 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Constant folding is executed for all runtimes during generation. It's used for folding long expressions with bitwise operations. Expressions like this:

if not(((((_la - 1)) & ~0x3f) == 0 and ((1 << (_la - 1)) & ((1 << (PParser.T0 - 1)) | (1 << (PParser.T1 - 1)) | (1 << (PParser.T2 - 1)) | (1 << (PParser.T3 - 1)) | (1 << (PParser.T4 - 1)) | (1 << (PParser.T5 - 1)) | (1 << (PParser.T6 - 1)) | (1 << (PParser.T7 - 1)) | (1 << (PParser.T8 - 1)) | (1 << (PParser.T9 - 1)) | (1 << (PParser.T10 - 1)) | (1 << (PParser.T11 - 1)) | (1 << (PParser.T12 - 1)) | (1 << (PParser.T13 - 1)) | (1 << (PParser.T14 - 1)) | (1 << (PParser.T15 - 1)) | (1 << (PParser.T16 - 1)) | (1 << (PParser.T17 - 1))

Constant propagation is used only for targets that don't support constant concept (Python, JavaScript), it differs from folding. For example, in the following code (it's not related to bitwise stuff):

self.enterRule(localctx, 0, self.RULE_r)

RULE_r will be replaced with constant literal:

self.enterRule(localctx, 0, 0)

but it will be saved for compilable languages. I thought it would be more clear without affecting performance, but we can use constant propagation for all targets as well (together with constant folding).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very interesting. Yes I can imagine in python that would be much much slower because it is doing symbol look up. Is there a way to distinguish between singletons like match(C.TYPEDEF) and sets as you have above? In other words how about we do your constant propagation but we limited to sets. Once you go beyond one token it's unlikely people are trying to read that for debugging purposes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to distinguish between singletons like match(C.TYPEDEF) and sets as you have above?

Yes, it's possible, but I implemented constant propagation for such cases because they are located in parser code and may affect the performance in theory as well. I can do it only for sets if you wish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, readability is important and match(T) should be left as-is. thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I managed to fix it without introducing the new member supportsConstant or similar.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests are failing, I'll take a look.

Go: getInlineTestSetWordSize 32 -> 64
Dart: get rid of BigInt
Swift: optimize TestSetInline
Python: fixes antlr#3698
JavaScript: fixes antlr#3699

Signed-off-by: Ivan Kochurkin <[email protected]>
Update getMultiTokenAlternativeDescriptor test

fixes antlr#3703

Signed-off-by: Ivan Kochurkin <[email protected]>
@KvanTTT
Copy link
Member Author

KvanTTT commented Aug 31, 2022

Eventually done. Some fails on C# tests, but are unrelated.

@parrt
Copy link
Member

parrt commented Aug 31, 2022

Eventually done. Some fails on C# tests, but are unrelated.

rerunning C# now...

@parrt parrt added the code-gen label Aug 31, 2022
@parrt
Copy link
Member

parrt commented Aug 31, 2022

Nice work! I'll merge it in and then verify the Java gen'd code still looks OK haha

@parrt parrt merged commit 6a2cd79 into antlr:dev Aug 31, 2022
@KvanTTT KvanTTT deleted the constant-folding branch August 31, 2022 21:43
@SimonStPeter
Copy link

SimonStPeter commented Oct 11, 2022 via email

@parrt
Copy link
Member

parrt commented Oct 22, 2022

@KvanTTT I have a warning from C++ where stuff like:

((_la & ~ 0x3fULL) == 0) &&
      ((1ULL << _la) & -41056) != 0 || (((_la - 64) & ~ 0x3fULL) == 0) &&
      ((1ULL << (_la - 64)) & 32767) != 0

gets warnings

$ clang++ -Werror,-Wlogical-op-parentheses t.cc
warning: unknown -Werror warning specifier: '-Werror,-Wlogical-op-parentheses' [-Wunknown-warning-option]
1 warning generated.
parrt-macbookpro2:tmp parrt$ clang++ -Werror -Wlogical-op-parentheses t.cc
t.cc:3:34: error: '&&' within '||' [-Werror,-Wlogical-op-parentheses]
    if (((_la & ~ 0x3fULL) == 0) &&
        ~~~~~~~~~~~~~~~~~~~~~~~~~^~
t.cc:3:34: note: place parentheses around the '&&' expression to silence this warning
    if (((_la & ~ 0x3fULL) == 0) &&
                                 ^

The -41056 makes me nervous. Are you sure that word size is portably correct, given the ULL usage?

@parrt
Copy link
Member

parrt commented Oct 22, 2022

Seems like C++ would change this template to wrap in parens:

bitsetBitfieldComparison(s, bits) ::= <<
<testShiftInRange({<offsetShift(s.varName, bits.shift)>})> &&
  ((1ULL \<\< <offsetShift(s.varName, bits.shift)>) & <bits.calculated>) != 0
>>

Some companies flip this warning to an error.

parrt added a commit to parrt/antlr4 that referenced this pull request Oct 22, 2022
…issues where compilers complain about not having parens around higher-precedence && vs || operators. A tweak to antlr#3701

Signed-off-by: Terence Parr <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants