diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel b/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel index 02fc857df..714ff3505 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/BUILD.bazel @@ -65,6 +65,7 @@ java_library( "//common:compiler_common", "//common:mutable_ast", "//common:mutable_source", + "//common:operator", "//common/ast", "//common/ast:cel_block", "//common/ast:mutable_expr", @@ -73,6 +74,7 @@ java_library( "//common/navigation:expr_util", "//common/navigation:mutable_navigation", "//common/types", + "//common/types:type_providers", "//extensions:bindings", "//optimizer:ast_optimizer", "//optimizer:mutable_ast", diff --git a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java index a53fdc7e9..fa1560e06 100644 --- a/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java +++ b/optimizer/src/main/java/dev/cel/optimizer/optimizers/SubexpressionOptimizer.java @@ -29,6 +29,7 @@ import com.google.common.base.Verify; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.collect.Multimaps; import com.google.common.collect.Sets; @@ -45,20 +46,29 @@ import dev.cel.common.CelSource.Extension.Version; import dev.cel.common.CelValidationException; import dev.cel.common.CelVarDecl; +import dev.cel.common.Operator; import dev.cel.common.ast.CelBlock; +import dev.cel.common.ast.CelConstant; import dev.cel.common.ast.CelExpr; import dev.cel.common.ast.CelExpr.CelCall; import dev.cel.common.ast.CelExpr.CelComprehension; import dev.cel.common.ast.CelExpr.CelList; import dev.cel.common.ast.CelExpr.ExprKind.Kind; import dev.cel.common.ast.CelMutableExpr; +import dev.cel.common.ast.CelMutableExpr.CelMutableCall; +import dev.cel.common.ast.CelMutableExpr.CelMutableComprehension; import dev.cel.common.ast.CelMutableExpr.CelMutableIdent; +import dev.cel.common.ast.CelMutableExpr.CelMutableList; +import dev.cel.common.ast.CelMutableExpr.CelMutableMap; +import dev.cel.common.ast.CelMutableExpr.CelMutableSelect; +import dev.cel.common.ast.CelMutableExpr.CelMutableStruct; import dev.cel.common.ast.CelMutableExprConverter; import dev.cel.common.navigation.CelNavigableExpr; import dev.cel.common.navigation.CelNavigableExprUtil; import dev.cel.common.navigation.CelNavigableMutableAst; import dev.cel.common.navigation.CelNavigableMutableExpr; import dev.cel.common.navigation.TraversalOrder; +import dev.cel.common.types.CelType; import dev.cel.common.types.SimpleType; import dev.cel.extensions.CelBindingsExtensions; import dev.cel.optimizer.AstMutator; @@ -69,8 +79,10 @@ import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.function.Predicate; import java.util.stream.Stream; @@ -159,6 +171,15 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel if (!cseOptions.populateMacroCalls()) { astToModify.source().clearMacroCalls(); } + ImmutableMap nodeTypes = ast.getTypeMap(); + + boolean simplified = simplifyDominatedSubexpressions(astToModify, nodeTypes); + CelAbstractSyntaxTree fallbackAst = + simplified + ? astMutator + .renumberIdsConsecutively(CelMutableAst.fromCelAst(astToModify.toParsedAst())) + .toParsedAst() + : ast; MangledComprehensionAst mangledComprehensionAst = astMutator.mangleComprehensionIdentifierNames( @@ -170,21 +191,19 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel CelMutableSource sourceToModify = astToModify.source(); int iterCount; - long nextId = 0; + long nextId = computeMaxId(astToModify); + if (cseOptions.subexpressionMaxRecursionDepth() == 0) { + nextId = maybeRewriteComplementaryNotEquals(astToModify, nodeTypes, nextId); + } ArrayList subexpressions = new ArrayList<>(); for (iterCount = 0; iterCount < cseOptions.iterationLimit(); iterCount++) { CelNavigableMutableAst navAst = CelNavigableMutableAst.fromAst(astToModify); - ImmutableList cseCandidates = getCseCandidates(navAst); + ValueNumberer valueNumberer = new ValueNumberer(navAst.getRoot().expr(), nodeTypes); + ImmutableList cseCandidates = getCseCandidates(navAst, valueNumberer); if (cseCandidates.isEmpty()) { break; } - if (iterCount == 0) { - nextId = navAst.getRoot().maxId(); - for (CelMutableExpr macroCall : sourceToModify.getMacroCalls().values()) { - nextId = max(nextId, CelNavigableMutableExpr.fromExpr(macroCall).maxId()); - } - } subexpressions.add(CelMutableExpr.newInstance(cseCandidates.get(0))); @@ -206,8 +225,7 @@ private OptimizationResult optimizeUsingCelBlock(CelAbstractSyntaxTree ast, Cel } if (iterCount == 0) { - // No modification has been made. - return OptimizationResult.create(ast); + return OptimizationResult.create(fallbackAst); } ImmutableList.Builder newVarDecls = ImmutableList.builder(); @@ -392,12 +410,21 @@ private static void replaceWithBlockIdent( expr.setIdent(CelMutableIdent.create(blockIdentifier)); } - private ImmutableList getCseCandidates(CelNavigableMutableAst navAst) { + private static long computeMaxId(CelMutableAst ast) { + long maxId = CelNavigableMutableAst.fromAst(ast).getRoot().maxId(); + for (CelMutableExpr macroCall : ast.source().getMacroCalls().values()) { + maxId = max(maxId, CelNavigableMutableExpr.fromExpr(macroCall).maxId()); + } + return maxId; + } + + private ImmutableList getCseCandidates( + CelNavigableMutableAst navAst, ValueNumberer valueNumberer) { if (cseOptions.subexpressionMaxRecursionDepth() > 0) { return getCseCandidatesWithRecursionDepth( - navAst, cseOptions.subexpressionMaxRecursionDepth()); + navAst, cseOptions.subexpressionMaxRecursionDepth(), valueNumberer); } else { - return getCseCandidatesWithCommonSubexpr(navAst); + return getCseCandidatesWithCommonSubexpr(navAst, valueNumberer); } } @@ -406,7 +433,7 @@ private ImmutableList getCseCandidates(CelNavigableMutableAst na * subexpr found. */ private ImmutableList getCseCandidatesWithRecursionDepth( - CelNavigableMutableAst navAst, int recursionLimit) { + CelNavigableMutableAst navAst, int recursionLimit, ValueNumberer valueNumberer) { checkArgument(recursionLimit > 0); Set ineligibleExprs = getIneligibleExprsFromComprehensionBranches(navAst); ImmutableList descendants = @@ -422,7 +449,7 @@ private ImmutableList getCseCandidatesWithRecursionDepth( } ImmutableList cseCandidates = - getCseCandidatesWithCommonSubexpr(descendants, ineligibleExprs); + getCseCandidatesWithCommonSubexpr(descendants, ineligibleExprs, valueNumberer); if (!cseCandidates.isEmpty()) { return cseCandidates; } @@ -446,31 +473,32 @@ private ImmutableList getCseCandidatesWithRecursionDepth( } private ImmutableList getCseCandidatesWithCommonSubexpr( - CelNavigableMutableAst navAst) { + CelNavigableMutableAst navAst, ValueNumberer valueNumberer) { Set ineligibleExprs = getIneligibleExprsFromComprehensionBranches(navAst); ImmutableList allNodes = navAst.getRoot().allNodes(TraversalOrder.PRE_ORDER).collect(toImmutableList()); - return getCseCandidatesWithCommonSubexpr(allNodes, ineligibleExprs); + return getCseCandidatesWithCommonSubexpr(allNodes, ineligibleExprs, valueNumberer); } private ImmutableList getCseCandidatesWithCommonSubexpr( - ImmutableList allNodes, Set ineligibleExprs) { + ImmutableList allNodes, + Set ineligibleExprs, + ValueNumberer valueNumberer) { ImmutableListMultimap nodesByHeight = Multimaps.index(allNodes, CelNavigableMutableExpr::height); - HashSet semanticallyEqualNodes = new HashSet<>(); + Set seenVns = new HashSet<>(); for (CelNavigableMutableExpr node : allNodes) { ImmutableList sameHeightNodes = nodesByHeight.get(node.height()); if (sameHeightNodes.size() < 2 || !canEliminate(node, ineligibleExprs)) { continue; } - // Normalize the expr to test semantic equivalence. - CelMutableExpr normalizedExpr = normalizeForEquality(node.expr()); - if (!semanticallyEqualNodes.add(normalizedExpr)) { + int vn = valueNumberer.vnOf(node.expr()); + if (!seenVns.add(vn)) { return sameHeightNodes.stream() .filter(candidate -> canEliminate(candidate, ineligibleExprs)) .map(CelNavigableMutableExpr::expr) - .filter(expr -> normalizeForEquality(expr).equals(normalizedExpr)) + .filter(expr -> valueNumberer.vnOf(expr) == vn) .collect(toImmutableList()); } } @@ -478,6 +506,63 @@ private ImmutableList getCseCandidatesWithCommonSubexpr( return ImmutableList.of(); } + // Rewrites eligible _!=_(a, b) calls in-place to !(_==_(a, b)) when an eligible _==_(a, b) or + // _==_(b, a) is also present in the AST, enabling CSE to share a single _==_ block slot. + private long maybeRewriteComplementaryNotEquals( + CelMutableAst astToModify, ImmutableMap nodeTypes, long nextId) { + CelNavigableMutableAst navAst = CelNavigableMutableAst.fromAst(astToModify); + ValueNumberer valueNumberer = new ValueNumberer(navAst.getRoot().expr(), nodeTypes); + Set ineligibleExprs = getIneligibleExprsFromComprehensionBranches(navAst); + ImmutableList allNodes = + navAst.getRoot().allNodes(TraversalOrder.PRE_ORDER).collect(toImmutableList()); + Set equalsVns = new HashSet<>(); + List notEqualsNodes = new ArrayList<>(); + for (CelNavigableMutableExpr node : allNodes) { + if (node.getKind() != Kind.CALL || !canEliminate(node, ineligibleExprs)) { + continue; + } + String func = node.expr().call().function(); + if (func.equals(Operator.EQUALS.getFunction())) { + equalsVns.add(valueNumberer.vnOf(node.expr())); + } else if (func.equals(Operator.NOT_EQUALS.getFunction())) { + notEqualsNodes.add(node); + } + } + for (CelNavigableMutableExpr node : notEqualsNodes) { + CelMutableCall call = node.expr().call(); + int eqVn = + valueNumberer.binaryCallVn( + Operator.EQUALS.getFunction(), + valueNumberer.vnOf(call.args().get(0)), + valueNumberer.vnOf(call.args().get(1))); + if (!equalsVns.contains(eqVn)) { + continue; + } + CelMutableExpr innerEquals = + CelMutableExpr.ofCall( + ++nextId, + CelMutableCall.create( + Operator.EQUALS.getFunction(), call.args().get(0), call.args().get(1))); + node.expr().setCall(CelMutableCall.create(Operator.LOGICAL_NOT.getFunction(), innerEquals)); + } + return nextId; + } + + private boolean simplifyDominatedSubexpressions( + CelMutableAst astToModify, ImmutableMap nodeTypes) { + ValueNumberer valueNumberer = new ValueNumberer(astToModify.expr(), nodeTypes); + DominatorSimplifier simplifier = + new DominatorSimplifier(valueNumberer, nodeTypes, astToModify.source()); + simplifier.simplifySubtree(astToModify.expr()); + return simplifier.modCount > 0; + } + + private static boolean isPotentiallyFloat( + CelMutableExpr expr, ImmutableMap nodeTypes) { + CelType type = nodeTypes.getOrDefault(expr.id(), SimpleType.DYN); + return type.equals(SimpleType.DOUBLE) || type.equals(SimpleType.DYN); + } + private boolean canEliminate( CelNavigableMutableExpr navigableExpr, Set ineligibleExprs) { return !navigableExpr.getKind().equals(Kind.CONSTANT) @@ -550,17 +635,365 @@ private boolean containsEliminableFunctionOnly(CelNavigableMutableExpr navigable }); } - /** - * Converts the {@link CelMutableExpr} to make it suitable for performing a semantically equals - * check. - * - *

Specifically, this will deep copy the mutable expr then set all expr IDs in the expression - * tree to 0. - */ - private CelMutableExpr normalizeForEquality(CelMutableExpr mutableExpr) { - CelMutableExpr copiedExpr = CelMutableExpr.newInstance(mutableExpr); + // Simplifies subexpressions using path-sensitive boolean predicate facts established by + // dominating ternary conditions. + private final class DominatorSimplifier { + private final ValueNumberer valueNumberer; + private final ImmutableMap nodeTypes; + private final CelMutableSource source; + private final Map knownBoolByVn; + private int modCount; + + private CelMutableExpr simplifySubtree(CelMutableExpr expr) { + if (expr.getKind() != Kind.CONSTANT) { + Boolean knownBool = knownBoolByVn.get(valueNumberer.vnOf(expr)); + if (knownBool != null) { + source.clearMacroCall(expr.id()); + expr.setConstant(CelConstant.ofValue(knownBool)); + modCount++; + return expr; + } + } + + int prevModCount = modCount; + switch (expr.getKind()) { + case CALL: + simplifyCallChildren(expr); + break; + case SELECT: + expr.select().setOperand(simplifySubtree(expr.select().operand())); + break; + case LIST: + expr.list().elements().replaceAll(this::simplifySubtree); + break; + case MAP: + for (CelMutableMap.Entry entry : expr.map().entries()) { + entry.setKey(simplifySubtree(entry.key())); + entry.setValue(simplifySubtree(entry.value())); + } + break; + case STRUCT: + for (CelMutableStruct.Entry entry : expr.struct().entries()) { + entry.setValue(simplifySubtree(entry.value())); + } + break; + case COMPREHENSION: + CelMutableComprehension comp = expr.comprehension(); + comp.setIterRange(simplifySubtree(comp.iterRange())); + if (comp.loopStep().getKind() == Kind.CALL) { + simplifyCallChildren(comp.loopStep()); + } + break; + default: + break; + } + + if (modCount != prevModCount && expr.getKind() == Kind.CALL) { + return foldModifiedCall(expr); + } + return expr; + } + + private void simplifyCallChildren(CelMutableExpr expr) { + CelMutableCall call = expr.call(); + List args = call.args(); + if (call.function().equals(Operator.CONDITIONAL.getFunction())) { + CelMutableExpr cond = simplifySubtree(args.get(0)); + args.set(0, cond); + Map savedBools = new HashMap<>(knownBoolByVn); + + recordTruth(cond, true); + args.set(1, simplifySubtree(args.get(1))); + + knownBoolByVn.clear(); + knownBoolByVn.putAll(savedBools); + + recordTruth(cond, false); + args.set(2, simplifySubtree(args.get(2))); + + knownBoolByVn.clear(); + knownBoolByVn.putAll(savedBools); + return; + } + call.target().ifPresent(target -> call.setTarget(simplifySubtree(target))); + args.replaceAll(this::simplifySubtree); + } + + private CelMutableExpr foldModifiedCall(CelMutableExpr expr) { + CelMutableCall call = expr.call(); + if (!call.function().equals(Operator.CONDITIONAL.getFunction())) { + return expr; + } + List args = call.args(); + CelMutableExpr cond = args.get(0); + if (cond.getKind() != Kind.CONSTANT) { + return expr; + } + return cond.constant().booleanValue() ? args.get(1) : args.get(2); + } + + private void recordTruth(CelMutableExpr expr, boolean isTrue) { + if (expr.getKind() == Kind.CALL) { + CelMutableCall call = expr.call(); + String func = call.function(); + List args = call.args(); + if (func.equals(Operator.LOGICAL_NOT.getFunction())) { + recordTruth(args.get(0), !isTrue); + return; + } + if (func.equals((isTrue ? Operator.LOGICAL_AND : Operator.LOGICAL_OR).getFunction())) { + args.forEach(arg -> recordTruth(arg, isTrue)); + return; + } + } + + if (!containsEliminableFunctionOnly(CelNavigableMutableExpr.fromExpr(expr))) { + return; + } + + int vn = valueNumberer.vnOf(expr); + knownBoolByVn.put(vn, isTrue); + + if (expr.getKind() != Kind.CALL) { + return; + } + CelMutableCall call = expr.call(); + String func = call.function(); + List args = call.args(); + if (func.equals(Operator.EQUALS.getFunction()) + || func.equals(Operator.NOT_EQUALS.getFunction())) { + boolean isEqualsOp = func.equals(Operator.EQUALS.getFunction()); + recordBinaryBool( + (isEqualsOp ? Operator.NOT_EQUALS : Operator.EQUALS).getFunction(), + valueNumberer.vnOf(args.get(0)), + valueNumberer.vnOf(args.get(1)), + !isTrue); + return; + } - return astMutator.clearExprIds(copiedExpr); + boolean isStrict; + boolean swapOperands; + if (func.equals(Operator.LESS.getFunction())) { + isStrict = true; + swapOperands = false; + } else if (func.equals(Operator.GREATER.getFunction())) { + isStrict = true; + swapOperands = true; + } else if (func.equals(Operator.LESS_EQUALS.getFunction())) { + isStrict = false; + swapOperands = false; + } else if (func.equals(Operator.GREATER_EQUALS.getFunction())) { + isStrict = false; + swapOperands = true; + } else { + return; + } + + CelMutableExpr lhs = args.get(0); + CelMutableExpr rhs = args.get(1); + int lhsVn = valueNumberer.vnOf(lhs); + int rhsVn = valueNumberer.vnOf(rhs); + int aVn = swapOperands ? rhsVn : lhsVn; + int bVn = swapOperands ? lhsVn : rhsVn; + if (isTrue) { + recordOrderFacts(aVn, bVn, isStrict); + } else if (!isPotentiallyFloat(lhs, nodeTypes) && !isPotentiallyFloat(rhs, nodeTypes)) { + // Under a total order (no NaN), !(a < b) <=> b <= a and !(a <= b) <=> b < a. + recordOrderFacts(bVn, aVn, !isStrict); + } + } + + private void recordOrderFacts(int aVn, int bVn, boolean isStrict) { + // a < b (or a <= b) == true guarantees neither operand is NaN. + recordBinaryBool(Operator.LESS_EQUALS.getFunction(), aVn, bVn, true); + recordBinaryBool(Operator.LESS.getFunction(), bVn, aVn, false); + if (isStrict) { + recordBinaryBool(Operator.LESS.getFunction(), aVn, bVn, true); + recordBinaryBool(Operator.LESS_EQUALS.getFunction(), bVn, aVn, false); + recordBinaryBool(Operator.EQUALS.getFunction(), aVn, bVn, false); + recordBinaryBool(Operator.NOT_EQUALS.getFunction(), aVn, bVn, true); + } + } + + private void recordBinaryBool(String func, int arg0Vn, int arg1Vn, boolean value) { + knownBoolByVn.put(valueNumberer.binaryCallVn(func, arg0Vn, arg1Vn), value); + } + + private DominatorSimplifier( + ValueNumberer valueNumberer, + ImmutableMap nodeTypes, + CelMutableSource source) { + this.valueNumberer = valueNumberer; + this.nodeTypes = nodeTypes; + this.source = source; + this.knownBoolByVn = new HashMap<>(); + this.modCount = 0; + } + } + + // Bottom-up hash-consing value numberer with algebraic canonicalization for commutative and + // converse operators and De Bruijn level scoping for comprehension variables. + private static final class ValueNumberer { + private static final ImmutableSet ALWAYS_COMMUTATIVE_FUNCTIONS = + ImmutableSet.of( + Operator.EQUALS.getFunction(), + Operator.NOT_EQUALS.getFunction(), + Operator.LOGICAL_AND.getFunction(), + Operator.LOGICAL_OR.getFunction()); + private static final ImmutableSet NUMERIC_TYPES = + ImmutableSet.of(SimpleType.INT, SimpleType.UINT, SimpleType.DOUBLE); + private static final ImmutableSet COMMUTATIVE_ADD_TYPES = + ImmutableSet.of( + SimpleType.INT, + SimpleType.UINT, + SimpleType.DOUBLE, + SimpleType.DURATION, + SimpleType.TIMESTAMP); + + private final ImmutableMap nodeTypes; + private final Map keyToVn; + private final IdentityHashMap exprToVn; + private ImmutableSet scopedVars; + + private int vnOf(CelMutableExpr expr) { + return checkNotNull(exprToVn.get(expr)); + } + + private int computeVn(CelMutableExpr expr) { + int vn = internKey(buildNodeKey(expr)); + exprToVn.put(expr, vn); + return vn; + } + + private int binaryCallVn(String function, int arg0Vn, int arg1Vn) { + return internKey(buildBinaryCallKey(function, arg0Vn, arg1Vn, SimpleType.BOOL)); + } + + private int internKey(Object key) { + return keyToVn.computeIfAbsent(key, k -> keyToVn.size()); + } + + private Object buildNodeKey(CelMutableExpr expr) { + switch (expr.getKind()) { + case CONSTANT: + return ImmutableList.of(Kind.CONSTANT, expr.constant()); + case IDENT: + String name = expr.ident().name(); + return ImmutableList.of(Kind.IDENT, name, scopedVars.contains(name)); + case SELECT: + CelMutableSelect select = expr.select(); + return ImmutableList.of( + Kind.SELECT, computeVn(select.operand()), select.field(), select.testOnly()); + case CALL: + return buildCallKey(expr); + case LIST: + CelMutableList list = expr.list(); + ImmutableList.Builder elemVns = + ImmutableList.builderWithExpectedSize(list.elements().size()); + for (CelMutableExpr elem : list.elements()) { + elemVns.add(computeVn(elem)); + } + return ImmutableList.of(Kind.LIST, elemVns.build(), list.optionalIndices()); + case MAP: + CelMutableMap map = expr.map(); + ImmutableList.Builder entryVns = + ImmutableList.builderWithExpectedSize(map.entries().size()); + for (CelMutableMap.Entry entry : map.entries()) { + entryVns.add( + ImmutableList.of( + computeVn(entry.key()), computeVn(entry.value()), entry.optionalEntry())); + } + return ImmutableList.of(Kind.MAP, entryVns.build()); + case STRUCT: + CelMutableStruct struct = expr.struct(); + ImmutableList.Builder fieldVns = + ImmutableList.builderWithExpectedSize(struct.entries().size()); + for (CelMutableStruct.Entry entry : struct.entries()) { + fieldVns.add( + ImmutableList.of( + entry.fieldKey(), computeVn(entry.value()), entry.optionalEntry())); + } + return ImmutableList.of(Kind.STRUCT, struct.messageName(), fieldVns.build()); + case COMPREHENSION: + CelMutableComprehension comp = expr.comprehension(); + int iterRangeVn = computeVn(comp.iterRange()); + int accuInitVn = computeVn(comp.accuInit()); + ImmutableSet savedScope = scopedVars; + scopedVars = ImmutableSet.of(comp.accuVar(), comp.iterVar(), comp.iterVar2()); + int loopConditionVn = computeVn(comp.loopCondition()); + int loopStepVn = computeVn(comp.loopStep()); + int resultVn = computeVn(comp.result()); + scopedVars = savedScope; + return ImmutableList.of( + Kind.COMPREHENSION, + comp.iterVar(), + comp.iterVar2(), + comp.accuVar(), + iterRangeVn, + accuInitVn, + loopConditionVn, + loopStepVn, + resultVn); + default: + throw new IllegalArgumentException("Unexpected ExprKind: " + expr.getKind()); + } + } + + private Object buildCallKey(CelMutableExpr expr) { + CelMutableCall call = expr.call(); + String function = call.function(); + List args = call.args(); + if (!call.target().isPresent() && args.size() == 2) { + return buildBinaryCallKey( + function, + computeVn(args.get(0)), + computeVn(args.get(1)), + nodeTypes.getOrDefault(expr.id(), SimpleType.DYN)); + } + Optional targetVn = call.target().map(this::computeVn); + ImmutableList.Builder argVns = ImmutableList.builderWithExpectedSize(args.size()); + for (CelMutableExpr arg : args) { + argVns.add(computeVn(arg)); + } + return ImmutableList.of(Kind.CALL, targetVn, function, argVns.build()); + } + + private static Object buildBinaryCallKey( + String function, int arg0Vn, int arg1Vn, CelType exprType) { + String canonicalFunc = function; + int firstVn = arg0Vn; + int secondVn = arg1Vn; + if (function.equals(Operator.GREATER.getFunction())) { + canonicalFunc = Operator.LESS.getFunction(); + firstVn = arg1Vn; + secondVn = arg0Vn; + } else if (function.equals(Operator.GREATER_EQUALS.getFunction())) { + canonicalFunc = Operator.LESS_EQUALS.getFunction(); + firstVn = arg1Vn; + secondVn = arg0Vn; + } else if (isCommutativeBinaryCall(function, exprType) && firstVn > secondVn) { + firstVn = arg1Vn; + secondVn = arg0Vn; + } + return ImmutableList.of( + Kind.CALL, Optional.empty(), canonicalFunc, ImmutableList.of(firstVn, secondVn)); + } + + private static boolean isCommutativeBinaryCall(String function, CelType exprType) { + return ALWAYS_COMMUTATIVE_FUNCTIONS.contains(function) + || (function.equals(Operator.MULTIPLY.getFunction()) && NUMERIC_TYPES.contains(exprType)) + || (function.equals(Operator.ADD.getFunction()) + && COMMUTATIVE_ADD_TYPES.contains(exprType)); + } + + @SuppressWarnings("ValueClassIdentity") // Only keys (CelMutableExpr) use reference identity + private ValueNumberer(CelMutableExpr root, ImmutableMap nodeTypes) { + this.nodeTypes = nodeTypes; + this.keyToVn = new HashMap<>(); + this.exprToVn = new IdentityHashMap<>(); + this.scopedVars = ImmutableSet.of(); + computeVn(root); + } } /** Options to configure how Common Subexpression Elimination behave. */ diff --git a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java index cba16b6df..934b2148b 100644 --- a/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java +++ b/optimizer/src/test/java/dev/cel/optimizer/optimizers/SubexpressionOptimizerTest.java @@ -79,12 +79,17 @@ private static Cel setupCelEnv(CelBuilder celBuilder) { .populateMacroCalls(true) .enableHeterogeneousNumericComparisons(true) .build()) - .addCompilerLibraries(CelExtensions.bindings(), CelExtensions.strings()) - .addRuntimeLibraries(CelExtensions.strings()) + .addCompilerLibraries( + CelExtensions.bindings(), CelExtensions.strings(), CelExtensions.optional()) + .addRuntimeLibraries(CelExtensions.strings(), CelExtensions.optional()) .addFunctionDeclarations( CelFunctionDecl.newFunctionDeclaration( "non_pure_custom_func", newGlobalOverload("non_pure_custom_func_overload", SimpleType.INT, SimpleType.INT))) + .addFunctionBindings( + CelFunctionBinding.fromOverloads( + "non_pure_custom_func", + CelFunctionBinding.from("non_pure_custom_func_overload", Long.class, val -> val))) .addVar("x", SimpleType.DYN) .addVar("msg", StructTypeReference.create(TestAllTypes.getDescriptor().getFullName())) .build(); @@ -822,6 +827,200 @@ public void optimize_afterSelectOptimizer_throwsException( .contains("SubexpressionOptimizer must run before SelectOptimizer."); } + private enum ValueNumberingTestCase { + COMMUTATIVE_INT_ADD( + "(msg.single_int64 + msg.single_int32) * (msg.single_int32 + msg.single_int64)", + "cel.@block([msg.single_int64 + msg.single_int32], @index0 * @index0)"), + COMMUTATIVE_UINT_MULTIPLY( + "(msg.single_uint64 * msg.single_uint32) + (msg.single_uint32 * msg.single_uint64)", + "cel.@block([msg.single_uint64 * msg.single_uint32], @index0 + @index0)"), + COMMUTATIVE_DOUBLE_ADD( + "(msg.single_double + 1.5) * (1.5 + msg.single_double)", + "cel.@block([msg.single_double + 1.5], @index0 * @index0)"), + COMMUTATIVE_DURATION_TIMESTAMP_ADD( + "(msg.single_duration + msg.single_timestamp) == (msg.single_timestamp +" + + " msg.single_duration)", + "cel.@block([msg.single_duration + msg.single_timestamp], @index0 == @index0)"), + NON_COMMUTATIVE_INT_SUBTRACT_PRESERVED( + "(msg.single_int64 - msg.single_int32) == (msg.single_int32 - msg.single_int64)", + "cel.@block([msg.single_int32, msg.single_int64], @index1 - @index0 == @index0 - @index1)"), + NON_COMMUTATIVE_STRING_CONCAT_PRESERVED( + "(msg.single_string + \"a\") == (\"a\" + msg.single_string)", + "cel.@block([msg.single_string], @index0 + \"a\" == \"a\" + @index0)"), + NON_COMMUTATIVE_BYTES_CONCAT_PRESERVED( + "(msg.single_bytes + b\"a\") == (b\"a\" + msg.single_bytes)", + "cel.@block([msg.single_bytes], @index0 + b\"\\141\" == b\"\\141\" + @index0)"), + NON_COMMUTATIVE_LIST_CONCAT_PRESERVED( + "([msg.single_int64] + [1]) == ([1] + [msg.single_int64])", + "cel.@block([[1], [msg.single_int64]], @index1 + @index0 == @index0 + @index1)"), + NON_COMMUTATIVE_DYN_ADD_PRESERVED( + "(dyn(msg.single_string) + dyn(\"a\")) == (dyn(\"a\") + dyn(msg.single_string))", + "cel.@block([dyn(\"a\"), dyn(msg.single_string)], @index1 + @index0 == @index0 +" + + " @index1)"), + COMMUTATIVE_EQUALS( + "(msg.single_int64 == 1) && (1 == msg.single_int64)", + "cel.@block([msg.single_int64 == 1], @index0 && @index0)"), + COMMUTATIVE_NOT_EQUALS( + "(msg.single_int64 != 1) || (1 != msg.single_int64)", + "cel.@block([msg.single_int64 != 1], @index0 || @index0)"), + COMMUTATIVE_LOGICAL_AND( + "((msg.single_int64 == 1) && (msg.single_int32 == 2)) || ((msg.single_int32 == 2) &&" + + " (msg.single_int64 == 1))", + "cel.@block([msg.single_int64 == 1 && msg.single_int32 == 2], @index0 || @index0)"), + RELATIONAL_CONVERSE_LESS_GREATER( + "(msg.single_int64 < msg.single_int32) && (msg.single_int32 > msg.single_int64)", + "cel.@block([msg.single_int64 < msg.single_int32], @index0 && @index0)"), + RELATIONAL_CONVERSE_LESS_EQUALS_GREATER_EQUALS( + "(msg.single_int64 <= msg.single_int32) && (msg.single_int32 >= msg.single_int64)", + "cel.@block([msg.single_int64 <= msg.single_int32], @index0 && @index0)"), + COMPLEMENTARY_EQUALS_NOT_EQUALS( + "(msg.single_int64 == 1 ? \"a\" : \"b\") + (msg.single_int64 != 1 ? \"c\" : \"d\")", + "cel.@block([msg.single_int64 == 1], (@index0 ? \"a\" : \"b\") + (!@index0 ? \"c\" :" + + " \"d\"))"), + COMPLEMENTARY_AND_COMMUTATIVE_EQUALS_NOT_EQUALS( + "(msg.single_int64 == 1 ? \"a\" : \"b\") + (1 != msg.single_int64 ? \"c\" : \"d\")", + "cel.@block([msg.single_int64 == 1], (@index0 ? \"a\" : \"b\") + (!@index0 ? \"c\" :" + + " \"d\"))"), + COMPLEMENTARY_NOT_EQUALS_CASCADING_PARENT_CSE( + "(!(msg.single_int64 == 1) && msg.single_int32 == 2) || (msg.single_int64 != 1 &&" + + " msg.single_int32 == 2)", + "cel.@block([!(msg.single_int64 == 1) && msg.single_int32 == 2], @index0 || @index0)"), + OPTIONAL_LIST_AND_MAP_DISTINCT_VALUE_NUMBERS( + "([?optional.of(msg.single_int64)].size() == [optional.of(msg.single_int64)].size()) &&" + + " ({?\"k\": optional.of(msg.single_int64)}.size() == {\"k\":" + + " optional.of(msg.single_int64)}.size())", + "cel.@block([optional.of(msg.single_int64)], [?@index0].size() == [@index0].size() &&" + + " {?\"k\": @index0}.size() == {\"k\": @index0}.size())"), + DOMINATED_TERNARY_POSITIVE_AND_NEGATIVE_BRANCHES( + "msg.single_int64 == 1 ? (msg.single_int64 == 1 ? \"a\" : \"b\") : (msg.single_int64 == 1" + + " ? \"c\" : \"d\")", + "(msg.single_int64 == 1) ? \"a\" : \"d\""), + DOMINATED_TERNARY_COMMUTATIVE_COMPLEMENTARY_BRANCHES( + "(msg.single_int64 == 1 ? (1 != msg.single_int64 ? \"a\" : \"b\") : (1 != msg.single_int64" + + " ? \"c\" : \"d\")) + (msg.single_int32 != 2 ? (2 == msg.single_int32 ? \"e\" :" + + " \"f\") : (2 == msg.single_int32 ? \"g\" : \"h\"))", + "((msg.single_int64 == 1) ? \"b\" : \"c\") + ((msg.single_int32 != 2) ? \"f\" : \"g\")"), + DOMINATED_TERNARY_CONJUNCTION_CONDITION( + "(msg.single_int64 == 1 && msg.single_int32 == 2) ? (msg.single_int64 == 1 ?" + + " (msg.single_int32 == 2 ? \"a\" : \"b\") : \"c\") : \"d\"", + "(msg.single_int64 == 1 && msg.single_int32 == 2) ? \"a\" : \"d\""), + DOMINATED_TERNARY_DISJUNCTION_FALSE_BRANCH( + "(msg.single_int64 == 1 || msg.single_int32 == 2) ? \"a\" : (msg.single_int64 == 1 ? \"b\"" + + " : (msg.single_int32 == 2 ? \"c\" : \"d\"))", + "(msg.single_int64 == 1 || msg.single_int32 == 2) ? \"a\" : \"d\""), + DOMINATED_TERNARY_NEGATED_CONDITION( + "!(msg.single_int64 == 1) ? (msg.single_int64 == 1 ? \"a\" : \"b\") : (msg.single_int64 ==" + + " 1 ? \"c\" : \"d\")", + "!(msg.single_int64 == 1) ? \"b\" : \"c\""), + DOMINATED_TERNARY_STRICT_GREATER_IMPLIES_GREATER_EQUALS( + "msg.single_int64 > 0 ? (msg.single_int64 >= 0 ? \"non_neg\" : \"neg\") : \"other\"", + "(msg.single_int64 > 0) ? \"non_neg\" : \"other\""), + DOMINATED_TERNARY_TOTAL_ORDER_NOT_LESS_IMPLIES_GREATER_EQUALS( + "msg.single_int64 < 0 ? \"neg\" : (msg.single_int64 >= 0 ? \"non_neg\" : \"unreachable\")", + "(msg.single_int64 < 0) ? \"neg\" : \"non_neg\""), + DOMINATED_TERNARY_LESS_EQUALS_AND_GREATER_EQUALS( + "(msg.single_int64 <= 0 ? (msg.single_int64 > 0 ? \"a\" : \"b\") : \"c\") +" + + " (msg.single_int64 >= 0 ? \"d\" : (msg.single_int64 < 0 ? \"e\" : \"f\"))", + "cel.@block([msg.single_int64], ((@index0 <= 0) ? \"b\" : \"c\") + ((@index0 >= 0) ? \"d\"" + + " : \"e\"))"), + DOMINATED_TERNARY_STRICT_LESS_IMPLIES_NOT_EQUAL( + "msg.single_int64 < 0 ? (msg.single_int64 == 0 ? \"zero\" : (msg.single_int64 != 0 ?" + + " (msg.single_int64 >= 0 ? \"non_neg\" : \"neg\") : \"unreachable\")) : \"non_neg\"", + "(msg.single_int64 < 0) ? \"neg\" : \"non_neg\""), + DOMINATED_TERNARY_HAS_MACRO_AND_METHOD_TARGET( + "has(msg.single_int64) ? ((has(msg.single_int64) ? \"ab\" : \"\").startsWith(\"a\") &&" + + " (has(msg.single_int64) ? has(msg.single_int32) : false)) : false", + "has(msg.single_int64) ? (\"ab\".startsWith(\"a\") && has(msg.single_int32)) : false"), + DOMINATED_TERNARY_CONTAINERS_AND_SELECT( + "msg.single_int64 == 1 ? [{msg.single_int64 == 1:" + + " cel.expr.conformance.proto3.TestAllTypes{single_bool: (msg.single_int64 == 1 ? msg" + + " : msg).single_bool}}] : [{true:" + + " cel.expr.conformance.proto3.TestAllTypes{single_bool: false}}]", + "(msg.single_int64 == 1) ? [{true: cel.expr.conformance.proto3.TestAllTypes{single_bool:" + + " msg.single_bool}}] : [{true: cel.expr.conformance.proto3.TestAllTypes{single_bool:" + + " false}}]"), + DOMINATED_TERNARY_COMPREHENSION_PRESERVES_MACRO_CALLS( + "has(msg.single_int64) ? [has(msg.single_int64)].all(y, y && (has(msg.single_int64) ? y :" + + " false)) : false", + "has(msg.single_int64) ? [true].all(y, y && y) : false"), + DOMINATED_TERNARY_NON_PURE_FUNCTION_NOT_FOLDED( + "non_pure_custom_func(1) == 1 ? (non_pure_custom_func(1) == 1 ? \"a\" : \"b\") : \"c\"", + "(non_pure_custom_func(1) == 1) ? ((non_pure_custom_func(1) == 1) ? \"a\" : \"b\") :" + + " \"c\""), + DOMINATED_TERNARY_SHADOWED_COMPREHENSION_VAR_NOT_FOLDED( + "x == 5 ? ([1, 2].all(x, x == 5) && (x == 5 ? true : false)) : false", + "(x == 5) ? ([1, 2].all(x, x == 5) && true) : false"), + ; + + private final String source; + private final String expectedUnparsed; + + ValueNumberingTestCase(String source, String expectedUnparsed) { + this.source = source; + this.expectedUnparsed = expectedUnparsed; + } + } + + @Test + public void cse_valueNumberingAndDominatorSimplification( + @TestParameter ValueNumberingTestCase testCase) throws Exception { + CelAbstractSyntaxTree ast = cel.compile(testCase.source).getAst(); + CelOptimizer optimizer = + newCseOptimizer( + SubexpressionOptimizerOptions.newBuilder().populateMacroCalls(true).build()); + ImmutableMap input = + ImmutableMap.of( + "x", + 5L, + "msg", + TestAllTypes.newBuilder() + .setSingleInt64(1L) + .setSingleInt32(2) + .setSingleUint64(3L) + .setSingleUint32(4) + .setSingleDouble(2.5) + .setSingleString("hello") + .build()); + + CelAbstractSyntaxTree optimizedAst = optimizer.optimize(ast); + + assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(testCase.expectedUnparsed); + assertThat(cel.createProgram(optimizedAst).eval(input)) + .isEqualTo(cel.createProgram(ast).eval(input)); + } + + @Test + @TestParameters( + "{source: '(msg.single_double < 0.0) || (0.0 > msg.single_double)', expectedUnparsed:" + + " 'cel.@block([msg.single_double < 0.0], @index0 || @index0)'}") + @TestParameters( + "{source: '(msg.single_double == 0.0 ? \"a\" : \"b\") + (msg.single_double != 0.0 ? \"c\" :" + + " \"d\")', expectedUnparsed: 'cel.@block([msg.single_double == 0.0], (@index0 ? \"a\" :" + + " \"b\") + (!@index0 ? \"c\" : \"d\"))'}") + @TestParameters( + "{source: '0 > msg.single_double ? \"neg\" : (0 <= msg.single_double ? \"non_neg\" :" + + " \"nan\")', expectedUnparsed: 'cel.@block([msg.single_double], (0 > @index0) ?" + + " \"neg\" : ((0 <= @index0) ? \"non_neg\" : \"nan\"))'}") + @TestParameters( + "{source: 'dyn(msg.single_double) < 0 ? \"neg\" : (dyn(msg.single_double) >= 0 ? \"non_neg\"" + + " : \"nan\")', expectedUnparsed: 'cel.@block([dyn(msg.single_double)], (@index0 < 0) ?" + + " \"neg\" : ((@index0 >= 0) ? \"non_neg\" : \"nan\"))'}") + public void cse_floatingPointNaN_preservesSemantics(String source, String expectedUnparsed) + throws Exception { + CelAbstractSyntaxTree ast = cel.compile(source).getAst(); + CelOptimizer optimizer = + newCseOptimizer( + SubexpressionOptimizerOptions.newBuilder().populateMacroCalls(true).build()); + ImmutableMap nanInput = + ImmutableMap.of("msg", TestAllTypes.newBuilder().setSingleDouble(Double.NaN).build()); + + CelAbstractSyntaxTree optimizedAst = optimizer.optimize(ast); + + assertThat(CEL_UNPARSER.unparse(optimizedAst)).isEqualTo(expectedUnparsed); + assertThat(cel.createProgram(optimizedAst).eval(nanInput)) + .isEqualTo(cel.createProgram(ast).eval(nanInput)); + } + /** * Converts AST containing cel.block related test functions to internal functions (e.g: cel.block * -> cel.@block)