diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/FunctionSplitter.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/FunctionSplitter.java index e6faabb52..5071154a8 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/FunctionSplitter.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/FunctionSplitter.java @@ -38,8 +38,11 @@ private void optimize() { Preconditions.checkArgument(func.getReturnType() instanceof ImVoid, "func must return void"); // run some basic optimizations first: func.flatten(tr); - LocalPlayerContextAnalyzer localPlayerContextAnalyzer = - new LocalPlayerContextAnalyzer(tr.getImProg()); + // Both passes ask the locality analysis only about locals and the values assigned to them (the function has + // no parameters), and the compile-time state usually has none, so the analysis is made only when it has some. + LocalPlayerContextAnalyzer localPlayerContextAnalyzer = func.getLocals().isEmpty() + ? null + : new LocalPlayerContextAnalyzer(tr.getImProg()); new ConstantAndCopyPropagation().optimizeFunc(func, localPlayerContextAnalyzer); new LocalMerger().optimizeFunc(func, localPlayerContextAnalyzer, tr); Set usedVars = UsedVariables.calculate(func); diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalMerger.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalMerger.java index d57ca01a4..0ec7caff9 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalMerger.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalMerger.java @@ -119,6 +119,8 @@ private void mergeLocals(Map> livenessInfo, Set liveAt if (!canMerge(color.getType(), v.getType())) { continue; } + // A merged local would hold both values while the analysis, made before this pass, still answers for + // each: keeping them apart keeps it true for the passes which use it after this one. if (localPlayerContextAnalyzer != null && localPlayerContextAnalyzer.isLocalPlayerDependent(v) != localPlayerContextAnalyzer.isLocalPlayerDependent(color)) { diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalPlayerContextAnalyzer.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalPlayerContextAnalyzer.java index 16528cce9..41170777b 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalPlayerContextAnalyzer.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/intermediatelang/optimizer/LocalPlayerContextAnalyzer.java @@ -215,7 +215,8 @@ public boolean isLocalPlayerDependent(ImVar variable) { return variable != null && variableIsDependent(variable); } - public boolean isLocalPlayerSource(ImFunction function) { + /** Whether {@code function} is a native whose value may differ between clients; a fixed list, no analysis. */ + public static boolean isLocalPlayerSource(ImFunction function) { return isClientLocalValueSource(function); } diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imoptimizer/ImInliner.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imoptimizer/ImInliner.java index b1b6b39d3..2dfce5ba6 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imoptimizer/ImInliner.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imoptimizer/ImInliner.java @@ -433,7 +433,7 @@ private static boolean isStoreTarget(ImLExpr target) { private boolean isEffectFree(Element e) { if (e instanceof ImFunctionCall call) { ImFunction target = call.getFunc(); - if (!target.isNative() || localPlayerAnalysis().isLocalPlayerSource(target) + if (!target.isNative() || LocalPlayerContextAnalyzer.isLocalPlayerSource(target) || !(SideEffectFreeNatives.isFunctionWithoutSideEffect(target.getName()) || translator.isLuaTableRead(target))) { return false; diff --git a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaOldGenericsCasts.java b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaOldGenericsCasts.java index a650e55ff..1ee399d10 100644 --- a/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaOldGenericsCasts.java +++ b/de.peeeq.wurstscript/src/main/java/de/peeeq/wurstscript/translation/imtranslation/LuaOldGenericsCasts.java @@ -1,7 +1,6 @@ package de.peeeq.wurstscript.translation.imtranslation; import de.peeeq.wurstscript.intermediatelang.optimizer.LocalMerger; -import de.peeeq.wurstscript.intermediatelang.optimizer.LocalPlayerContextAnalyzer; import de.peeeq.wurstscript.jassIm.*; import de.peeeq.wurstscript.types.TypesHelper; @@ -24,6 +23,9 @@ * got some are passed through the local merger once more: the temporaries share a slot with each * other and with dead locals of their type, and a function with many casts does not grow towards * Lua's limit of 200 locals. Merging only renames variables, so the operands stay variables. + * The merger runs here without a locality analysis, so a client-local value and a synchronized one + * may share a local. Elsewhere it keeps them apart only so that the analysis made before it stays + * true for the passes which use it afterwards; this is the last pass, so nothing uses one after it. */ public final class LuaOldGenericsCasts { @@ -72,9 +74,8 @@ public void visit(ImCast cast) { replacement)); } prog.flatten(translator); - LocalPlayerContextAnalyzer analyzer = new LocalPlayerContextAnalyzer(prog); for (ImFunction function : touched) { - new LocalMerger().optimizeFunc(function, analyzer, translator); + new LocalMerger().optimizeFunc(function, null, translator); } } } diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaBackendAuditTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaBackendAuditTests.java index 3d28b23b1..ee7d3f05b 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaBackendAuditTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/LuaBackendAuditTests.java @@ -1311,6 +1311,41 @@ public void oldGenericsCastTemporariesAreMerged() { assertFalse("every cast is still inlined:\n" + readMany, readMany.contains("__wurst_oldGenericsFromInt")); } + /** + * The merge after the cast operands are hoisted is the last pass, so nothing reads a locality analysis + * after it, and a client-local local may share its slot with a synchronized one which is dead by then. + */ + @Test + public void oldGenericsCastMergeIgnoresLocality() { + String compiled = compileOptimizedLua("oldGenericsCastMergeIgnoresLocality", + "type player extends handle", + "package Test", + "@extern native GetLocalPlayer() returns player", + "@extern native GetPlayerId(player p) returns integer", + "native readInt() returns integer", + "native consume(integer value)", + "int array slots", + "@inline function load(int key) returns T", + " return slots[key] castTo T", + "@noinline function show(int key)", + " let id = GetPlayerId(GetLocalPlayer())", + " consume(id)", + " consume(id)", + " let other = readInt()", + " consume(other)", + " consume(other)", + " consume(load(key))", + "init", + " show(1)"); + String show = topLevelFunctionBodyWithPrefix(compiled, "show"); + java.util.regex.Matcher clientLocal = + java.util.regex.Pattern.compile("(\\w+) = GetPlayerId\\(GetLocalPlayer\\(\\)\\)").matcher(show); + assertTrue("the client-local value is kept in a local:\n" + show, clientLocal.find()); + String local = clientLocal.group(1); + assertTrue("that local is reused for a synchronized value:\n" + show, + show.contains(local + " = readInt()") || show.contains(local + " = Test_slots[key]")); + } + /** * A class-typed caller uses an old-generics result as is, so a missing entry has to come back * as nil: TimedLoop looks its timer up in a HashMap and creates one when the lookup is null. diff --git a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java index 30a900e8d..a43ae1cde 100644 --- a/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java +++ b/de.peeeq.wurstscript/src/test/java/tests/wurstscript/tests/OptimizerTests.java @@ -3934,35 +3934,12 @@ public void clientLocalNativeValuesAreLocalitySources() { "BlzIsSelectionEnabled" )); Element trace = Ast.NoExpr(); - ImFunctions functions = JassIm.ImFunctions(); - java.util.Map functionsByName = new java.util.LinkedHashMap<>(); - for (String name : localValueSources) { - ImFunction nativeFunction = nativeIntFunction(trace, name); - functions.add(nativeFunction); - functionsByName.put(name, nativeFunction); - } - for (String name : intentionallyExcludedSources) { - ImFunction nativeFunction = nativeIntFunction(trace, name); - functions.add(nativeFunction); - functionsByName.put(name, nativeFunction); - } - ImProg prog = JassIm.ImProg( - trace, - JassIm.ImVars(), - functions, - JassIm.ImMethods(), - JassIm.ImClasses(), - JassIm.ImTypeClassFuncs(), - new java.util.HashMap<>() - ); - LocalPlayerContextAnalyzer analyzer = new LocalPlayerContextAnalyzer(prog); - for (String name : localValueSources) { - assertTrue(analyzer.isLocalPlayerSource(functionsByName.get(name)), + assertTrue(LocalPlayerContextAnalyzer.isLocalPlayerSource(nativeIntFunction(trace, name)), name + " must be treated as a client-local value source"); } for (String name : intentionallyExcludedSources) { - assertFalse(analyzer.isLocalPlayerSource(functionsByName.get(name)), + assertFalse(LocalPlayerContextAnalyzer.isLocalPlayerSource(nativeIntFunction(trace, name)), name + " is synchronized event data or user-managed local state"); } }