Simplify boolean expressions in Java AST#10279
Simplify boolean expressions in Java AST#10279zbynek wants to merge 2 commits intogwtproject:mainfrom
Conversation
| } else { | ||
| // if side effect, allow rewriting a() && false && b() -> (a(), false) && b() | ||
| // -> (a(), false && b()) -> (a(), false) | ||
| return lhs.hasSideEffects() ? new JMultiExpression(info, lhs, rhs) : rhs; |
There was a problem hiding this comment.
to be clear, this is really just a() && false to (a(), false) right? Then on the way out we would handle the next stage (and test stuff address that so we know this change composes well with existing &&, ?: optimizations in DCE
For my part, I think it would read better if we had another else/if in here rather than a ternary within an else
| } else { | |
| // if side effect, allow rewriting a() && false && b() -> (a(), false) && b() | |
| // -> (a(), false && b()) -> (a(), false) | |
| return lhs.hasSideEffects() ? new JMultiExpression(info, lhs, rhs) : rhs; | |
| } else if (!lhs.hasSideEffects()) { | |
| return rhs; | |
| } else { | |
| // if side effect, allow rewriting a() && false -> (a(), false) | |
| // which will enable other optimizations to eval them separately | |
| return new JMultiExpression(info, lhs, rhs); |
| + "static boolean randomBooleanWithSideEffects() { createA(); return random() > .5;}" | ||
| + "static boolean booleanWithoutSideEffects() { return true;}" | ||
| + "static int arithmeticWithSideEffects() { createA(); return 4;}" | ||
| + "static native double random() /*-{ }-*/;" |
There was a problem hiding this comment.
Discussed in the other PR, but this can be simpler if you split to another test, or turn off runMethodInliner before your specific tests. All methods are assumed to have side effects by default in GWT, but once they are inlined away they might no longer have side effects. You can also cheat your way out of this not with JSNI, but just a @DoNotInline. It makes the tests easier to read/write if you don't have to mess with this stuff
| optimizeExpressions(false, "boolean", "false && A.booleanWithSideEffects()") | ||
| .intoString("return false;"); | ||
|
|
||
| optimizeExpressions(false, "int", "A.randomBooleanWithSideEffects() && false && A.randomBooleanWithSideEffects() ? 1 : 2") |
There was a problem hiding this comment.
Add a test for just plain && and || that fails before this patch - the more complex test is great to have as well, but we should test the simpler case.
|
Stats for GeoGebra main chunk before: after: So this seems to be just .03% improvement. |
Right, most of these are going to be tiny on their own, or will depend on the specific code being compiled repeating this pattern. I still think changes like this in pattern matching simple code is worth it - we're adding a single |
2 participants
Split from #10261 (see comments in that PR for discussion).
Fixes #10278