Skip to content

Commit

Permalink
Never attempt to inline switch expressions (#10006)
Browse files Browse the repository at this point in the history
The original check wasn't specific enough - couldn't handle nested
expressions, or expressions in different types of wrapper statements.

Also includes a fix preventing record equals methods from being made
static.

Fixes #10005
  • Loading branch information
niloc132 authored Oct 14, 2024
1 parent 5ecb770 commit 53606b5
Show file tree
Hide file tree
Showing 4 changed files with 71 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -193,13 +193,16 @@ private void implementEquals(JRecordType type, JMethod method, SourceInfo info)
myField,
otherField);
} else {
// we would like to use Objects.equals here to be more consise, but we would need
// We would like to use Objects.equals here to be more concise, but we would need
// to look up the right impl based on the field - just as simple to insert a null check
// and get it a little closer to all being inlined away

// Make another field ref to call equals() on
JFieldRef myField2 = new JFieldRef(info, new JThisRef(info, type), field, type);
equals = new JBinaryOperation(info, JPrimitiveType.BOOLEAN, JBinaryOperator.AND,
new JBinaryOperation(info, JPrimitiveType.BOOLEAN, JBinaryOperator.NEQ,
myField, JNullLiteral.INSTANCE),
new JMethodCall(info, myField, objectEquals, otherField));
new JMethodCall(info, myField2, objectEquals, otherField));
}
if (componentCheck != JBooleanLiteral.TRUE) {
componentCheck = new JBinaryOperation(info, JPrimitiveType.BOOLEAN,
Expand Down
37 changes: 34 additions & 3 deletions dev/core/src/com/google/gwt/dev/jjs/impl/MethodInliner.java
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,31 @@ public boolean visit(JThisRef x, Context ctx) {
}
}

/**
* Determines if the given expression can be inlined. Any switch expression will fail this check.
*/
private static class CannotBeInlinedVisitor extends JVisitor {
private boolean succeed = true;
public static boolean check(JExpression expr) {
CannotBeInlinedVisitor v = new CannotBeInlinedVisitor();
v.accept(expr);
return v.succeed;
}

@Override
public boolean visit(JStatement x, Context ctx) {
// To ensure we didn't miss an important case, throw if we see a statement, as those cannot
// be inlined.
throw new IllegalStateException("Should never visit statements");
}

@Override
public boolean visit(JSwitchExpression x, Context ctx) {
succeed = false;
return false;
}
}

/**
* Method inlining visitor.
*/
Expand Down Expand Up @@ -148,6 +173,7 @@ private InlineResult tryInlineMethodCall(JMethodCall x, Context ctx) {
if (expressions == null) {
// If it will never be possible to inline the method, add it to a
// blacklist

return InlineResult.BLACKLIST;
}

Expand Down Expand Up @@ -242,6 +268,9 @@ private List<JExpression> extractExpressionsFromBody(JMethodBody body) {
if (initializer == null) {
continue;
}
if (!CannotBeInlinedVisitor.check(initializer)) {
return null;
}
JLocal local = (JLocal) declStatement.getVariableRef().getTarget();
JExpression clone = new JBinaryOperation(stmt.getSourceInfo(), local.getType(),
JBinaryOperator.ASG,
Expand All @@ -251,17 +280,19 @@ private List<JExpression> extractExpressionsFromBody(JMethodBody body) {
} else if (stmt instanceof JExpressionStatement) {
JExpressionStatement exprStmt = (JExpressionStatement) stmt;
JExpression expr = exprStmt.getExpr();
if (expr instanceof JSwitchExpression) {
// Switch expressions can't be cloned in this way, though we wouldn't want to inline
// such a large block anyway.
if (!CannotBeInlinedVisitor.check(expr)) {
return null;
}
JExpression clone = cloner.cloneExpression(expr);
expressions.add(clone);
} else if (stmt instanceof JReturnStatement) {
JReturnStatement returnStatement = (JReturnStatement) stmt;
JExpression expr = returnStatement.getExpr();

if (expr != null) {
if (!CannotBeInlinedVisitor.check(expr)) {
return null;
}
JExpression clone = cloner.cloneExpression(expr);
clone = maybeCast(clone, body.getMethod().getType());
expressions.add(clone);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -439,4 +439,32 @@ public void testSwitchInSubExpr() {
: 4.0;
assertTrue(notCalled);
}

public void testSwitchExprInlining() {
enum HasSwitchMethod {
A, RED, SUNDAY, JANUARY, ZERO;
public static final int which(HasSwitchMethod whichSwitch) {
return switch(whichSwitch) {
case A -> 1;
case RED -> 2;
case SUNDAY -> 3;
case JANUARY -> 4;
case ZERO -> 5;
};
}
public static final int pick(HasSwitchMethod whichSwitch) {
return 2 * switch(whichSwitch) {
case A -> 1;
case RED -> 2;
case SUNDAY -> 3;
case JANUARY -> 4;
case ZERO -> 5;
};
}
}

HasSwitchMethod uninlinedValue = Math.random() > 2 ? HasSwitchMethod.A : HasSwitchMethod.RED;
assertEquals(2, HasSwitchMethod.which(uninlinedValue));
assertEquals(4, HasSwitchMethod.pick(uninlinedValue));
}
}
4 changes: 4 additions & 0 deletions user/test/com/google/gwt/dev/jjs/test/Java17Test.java
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,10 @@ public void testSwitchInSubExpr() {
assertFalse(isGwtSourceLevel17());
}

public void testSwitchExprInlining() {
assertFalse(isGwtSourceLevel17());
}

private boolean isGwtSourceLevel17() {
return JUnitShell.getCompilerOptions().getSourceLevel().compareTo(SourceLevel.JAVA17) >= 0;
}
Expand Down

0 comments on commit 53606b5

Please sign in to comment.