From c643d3411cd2f5caba2aacf9f69e4a000f9478c6 Mon Sep 17 00:00:00 2001 From: Eldritch Conundrum Date: Thu, 2 May 2024 14:13:54 -0700 Subject: [PATCH] Make unit tests more resilient before adding new rewrite (#366) Co-authored-by: Laurent Le Brun --- src/analyzer.fs | 20 +++++----- src/rewriter.fs | 61 ++++++++++++++++++++++++++++++ tests/commands.txt | 1 + tests/unit/float.frag | 16 +++++--- tests/unit/float.frag.expected | 15 +++++--- tests/unit/operators.expected | 3 +- tests/unit/operators.frag | 4 +- tests/unit/pi.frag | 3 ++ tests/unit/pi.frag.expected | 2 + tests/unit/reuse-var.frag | 37 ++++++++++++++++++ tests/unit/reuse-var.frag.expected | 36 ++++++++++++++++++ 11 files changed, 176 insertions(+), 22 deletions(-) create mode 100644 tests/unit/reuse-var.frag create mode 100644 tests/unit/reuse-var.frag.expected diff --git a/src/analyzer.fs b/src/analyzer.fs index 910cd9b1..c20ddc95 100644 --- a/src/analyzer.fs +++ b/src/analyzer.fs @@ -8,6 +8,14 @@ open Options.Globals // information in the AST nodes, e.g. find which variables are modified, // which declarations can be inlined. +let varUsesInStmt stmt = + let mutable idents = [] + let collectLocalUses _ = function + | Var v as e -> idents <- v :: idents; e + | e -> e + mapStmt BlockLevel.Unknown (mapEnvExpr collectLocalUses) stmt |> ignore + idents + module private VariableInlining = // Return the list of variables used in the statements, with the number of references. @@ -22,14 +30,6 @@ module private VariableInlining = mapStmt BlockLevel.Unknown (mapEnvExpr collectLocalUses) expr |> ignore counts - let collectReferencesSet expr = // INCORRECT: the shadowing variables are merged! they might hide a writing reference! - let result = HashSet() - let collectLocalUses _ = function - | Var v as e -> result.Add(v) |> ignore; e - | e -> e - mapExpr (mapEnvExpr collectLocalUses) expr |> ignore - result - let isEffectivelyConst (ident: Ident) = match ident.Declaration with | Declaration.Variable varDecl -> @@ -59,8 +59,8 @@ module private VariableInlining = | None -> () | Some init -> localExpr <- init :: localExpr - let deps = collectReferencesSet init - let isConst = deps |> Seq.forall isEffectivelyConst + let isConst = // INCORRECT: the shadowing variables are merged! they might hide a writing reference! + varUsesInStmt (Expr init) |> Seq.forall isEffectivelyConst localDefs.[def.name.Name] <- (def.name, isConst) | Expr e | Jump (_, Some e) -> localExpr <- e :: localExpr diff --git a/src/rewriter.fs b/src/rewriter.fs index c5b91582..c2cf1e0b 100644 --- a/src/rewriter.fs +++ b/src/rewriter.fs @@ -416,6 +416,61 @@ module private RewriterImpl = | Switch (_e, cases) -> cases |> List.forall (fun (_label, stmts) -> hasNoContinue stmts) | _ -> true) + // Reuse an existing local variable declaration that won't be used anymore, instead of introducing a new one. + // The reused identifier gets compressed better, and the declaration is sometimes removed. + // float d1 = f(); float d2 = g(); -> float d1 = f(); d1 = g(); + let reuseExistingVarDecl b = + let tryReplaceWithPrecedingAndFollowing f (xs : Stmt list) = + let rec go f preceding = function + | head :: tail -> + match f (preceding, head, tail) with + | None -> head :: go f (head :: preceding) tail // preceding is reversed, that's good + | Some xs -> xs @ tail + | [] -> [] + go f [] xs + b |> tryReplaceWithPrecedingAndFollowing (function + | (preceding2, Decl (ty2, declElts), following2) -> + let findAssignmentReplacementFor declElt2 = + let compatibleDeclElts = preceding2 |> List.collect (function + // The previous declaration must have the same type. + | Decl (ty1, declElts1) when ty2 = ty1 -> + let firstIsNotUsedAfterSecondDeclared (declElt1 : DeclElt) followingDecl2 = + // The first variable must not be used after the second is declared. + Analyzer.varUsesInStmt (Block followingDecl2) |> List.forall (fun i -> i.Name <> declElt1.name.Name) + declElts1 |> List.filter (fun declElt1 -> + // The previous declaration must have the same size and semantics + declElt1.size = declElt2.size && + declElt1.semantics = declElt2.semantics && + firstIsNotUsedAfterSecondDeclared declElt1 following2 + ) + | _ -> []) + match compatibleDeclElts |> List.tryHead with + | Some (declElt1) -> + debug $"{declElt2.name.Loc}: eliminating local variable '{declElt2.name}' by reusing existing local variable '{declElt1.name}'" + for v in Analyzer.varUsesInStmt (Block following2) do // Rename all uses of var2 to use var1 instead. + if v.Name = declElt2.name.Name then + v.Rename(declElt1.name.Name) + match declElt2.init with + | Some init -> Some [Expr (FunCall (Op "=", [Var declElt1.name; init]))] + | None -> Some [] + | _ -> None + match declElts, (declElts |> List.rev) with + //| [declElt2], _ -> // float d1=f(); ...; float d2=g(); -> ...; d1=g(); + // match findAssignmentReplacementFor declElt2 with + // | Some stmts -> Some stmts // Replace the Decl with the assignment. Largest win + // | None -> None + //| (declElt2 :: others), _ -> // float d1=f(); ...; float d2=g(),d3=h(); -> ...; d1=g(); float d3=h(); + // match findAssignmentReplacementFor declElt2 with + // | Some stmts -> Some (stmts @ [Decl (ty2, others)]) // Keep the Decl, add an assignment before it. + // | None -> None + //| _, (declElt2 :: others) -> // float d1=f(); ...; float d3=h(),d2=g(); -> ...; float d3=h(); d1=g(); + // match findAssignmentReplacementFor declElt2 with + // | Some stmts -> Some (Decl (ty2, others) :: stmts) // Keep the Decl, add an assignment after it. + // | None -> None + // For a decl sandwiched between others, we could consider moving the assignment into a comma-expr of the init of the next decl, but this adds parentheses. + | _ -> None + | _ -> None) + let simplifyBlock blockLevel stmts = let b = stmts // Avoid some optimizations when there are preprocessor directives. @@ -522,6 +577,12 @@ module private RewriterImpl = | stmts -> stmts let b = replaceIfReturnsWithReturnTernary b + let b = + // We ensure control flow analysis is trivial by only doing this at the root block level. + if blockLevel <> BlockLevel.FunctionRoot || hasPreprocessor + then b + else reuseExistingVarDecl b + // Consecutive declarations of the same type become one. float a;float b; -> float a,b; let b = squeezeConsecutiveDeclarations b diff --git a/tests/commands.txt b/tests/commands.txt index 19d94853..eb400f3a 100644 --- a/tests/commands.txt +++ b/tests/commands.txt @@ -32,6 +32,7 @@ --no-remove-unused --no-renaming --format indented -o tests/unit/precedence.frag.expected tests/unit/precedence.frag --no-remove-unused --format indented -o tests/unit/interface_block.frag.expected tests/unit/interface_block.frag --no-remove-unused --no-renaming --format indented -o tests/unit/ternary.frag.expected tests/unit/ternary.frag +--no-remove-unused --no-renaming --format indented -o tests/unit/reuse-var.frag.expected tests/unit/reuse-var.frag # Unused removal tests diff --git a/tests/unit/float.frag b/tests/unit/float.frag index 92f778b0..b3e65e8f 100644 --- a/tests/unit/float.frag +++ b/tests/unit/float.frag @@ -1,18 +1,20 @@ -void floatPrecision() { +float floatPrecision(float x) { float a = 0.00000012345; float b = 1.234567891234; float c = 1234567.891234; float d = 123456700000000.; + return a*x+b*x+c*x+d*x; } -void largeNumbers() { +float largeNumbers(float x) { float a = 4.1e8; float b = 4.2e10; float c = 4.3e12; float d = 4.4e14; + return a*x+b*x+c*x+d*x; } -void smallNumbers() { +float smallNumbers(float x) { float a = 4e-1; float b = 4e-2; float c = 4e-3; @@ -21,14 +23,18 @@ void smallNumbers() { float f = 4e-10; float g = 4e-12; float h = 4e-14; + return a*x+b*x+c*x+d*x+e*x+f*x+g*x+h*x; } -void zero() { +float zero(float x) { float a = 0.; float b = .0; float c = -.0; + return a*x+b*x+c*x; } +float x; + void main() { float f1 = 1.5; @@ -39,5 +45,5 @@ float f7 = 2E-9; float f8 = 2E+6; float f9 = 2e10; -gl_FragColor=vec4(0.); +gl_FragColor=vec4(f1*x+f2*x+f3*x+f6*x+f7*x+f8*x+f9*x); } diff --git a/tests/unit/float.frag.expected b/tests/unit/float.frag.expected index aad12784..fd80ddae 100644 --- a/tests/unit/float.frag.expected +++ b/tests/unit/float.frag.expected @@ -7,26 +7,31 @@ #else // if SHADER_MINIFIER_IMPL // tests/unit/float.frag - "void floatPrecision()" + "float floatPrecision(float x)" "{" "float a=1.2345e-7,b=1.234567891234,c=1234567.891234,d=1.234567e14;" + "return a*x+b*x+c*x+d*x;" "}" - "void largeNumbers()" + "float largeNumbers(float x)" "{" "float a=4.1e8,b=4.2e10,c=4.3e12,d=4.4e14;" + "return a*x+b*x+c*x+d*x;" "}" - "void smallNumbers()" + "float smallNumbers(float x)" "{" "float a=.4,b=.04,c=.004,d=4e-4,e=4e-8,f=4e-10,g=4e-12,h=4e-14;" + "return a*x+b*x+c*x+d*x+e*x+f*x+g*x+h*x;" "}" - "void zero()" + "float zero(float x)" "{" "float a=0.,b=0.,c=0.;" + "return a*x+b*x+c*x;" "}" + "float x;" "void main()" "{" "float f1=1.5,f2=3e-7,f3=.42,f6=-.002,f7=2e-9,f8=2e6,f9=2e10;" - "gl_FragColor=vec4(0);" + "gl_FragColor=vec4(f1*x+f2*x+f3*x+f6*x+f7*x+f8*x+f9*x);" "}", #endif diff --git a/tests/unit/operators.expected b/tests/unit/operators.expected index d197e3eb..a61946e4 100644 --- a/tests/unit/operators.expected +++ b/tests/unit/operators.expected @@ -8,10 +8,11 @@ // tests/unit/operators.frag "#version 330\n" - "void main()" + "float notmain()" "{" "int a=11,b=7,c=2;" "float f=1.4/3.,g=mod(8.,3.),z=122121;" + "return a+b+c+f+g+z;" "}" "int no_parens(int a,int b,int c)" "{" diff --git a/tests/unit/operators.frag b/tests/unit/operators.frag index b2847cb7..1b2474bc 100644 --- a/tests/unit/operators.frag +++ b/tests/unit/operators.frag @@ -1,6 +1,6 @@ #version 330 -void main() +float notmain() { int a = 1 + 2 * 3 + 4; int b = 14 / 2; @@ -18,6 +18,8 @@ void main() (1. >= 2. ? 1 : 2) * 1000 + (1. == 2. ? 1 : 2) * 10000 + (1. != 2. ? 1 : 2) * 100000; + + return a+b+c+f+g+z; } int no_parens(int a, int b, int c) { diff --git a/tests/unit/pi.frag b/tests/unit/pi.frag index 1c372fbf..d650550f 100644 --- a/tests/unit/pi.frag +++ b/tests/unit/pi.frag @@ -1,5 +1,7 @@ #version 400 +out vec4 fragColor; + void main() { double pi = 3.141592653589793; double minus_pi = -3.141592653589793; @@ -8,4 +10,5 @@ void main() { double half_pi = 1.5707963267948966; double minus_half_pi = -1.5707963267948966; double precise_pi = 3.14159265358979323846264338327950288419716939937510; + fragColor = vec4(pi+minus_pi+tau+minus_tau+half_pi+minus_half_pi+precise_pi); } diff --git a/tests/unit/pi.frag.expected b/tests/unit/pi.frag.expected index 060e8b39..9394485b 100644 --- a/tests/unit/pi.frag.expected +++ b/tests/unit/pi.frag.expected @@ -1,6 +1,8 @@ #version 400 +out vec4 fragColor; void main() { double pi=acos(-1.),minus_pi=-acos(-1.),tau=2.*acos(-1.),minus_tau=-2.*acos(-1.),half_pi=acos(0.),minus_half_pi=-acos(0.),precise_pi=acos(-1.); + fragColor=vec4(pi+minus_pi+tau+minus_tau+half_pi+minus_half_pi+precise_pi); } diff --git a/tests/unit/reuse-var.frag b/tests/unit/reuse-var.frag new file mode 100644 index 00000000..bd31af63 --- /dev/null +++ b/tests/unit/reuse-var.frag @@ -0,0 +1,37 @@ +float simple_var_decl_reuse(float x) +{ + float a = 1.+x; + int b = 3+int(x); + vec3 sep = vec3(0.); // prevents squeezeConsecutiveDeclarations + int b2 = 5+int(a); + float c = a; + sep += vec3(0.); // prevents squeezeConsecutiveDeclarations + float c2 = 9.+a; + int d = 3+b+b2; + sep += vec3(0.); // prevents squeezeConsecutiveDeclarations + int d2 = d+b2-b; + float e = c*c2; + sep += vec3(0.); // prevents squeezeConsecutiveDeclarations + float e2 = 4.-c-c2; + int f = 3*d*d2; + sep += vec3(0.); // prevents squeezeConsecutiveDeclarations + int f2 = 4/d2-d; + float g = e-float(f)+e2; + int g2 = int(4.-g+e2+e); + int h = 3*f-f2; + sep += vec3(0.); // prevents squeezeConsecutiveDeclarations + int h2 = 7*f2-f; + return length(sep)+float(h*h2)*g*float(g2)*x + float(g2/h2-h); +} +float multidecl_var_decl_reuse(float x) +{ + float a = 1.+x; + int b = 3+int(x), b2 = 5+int(a); + float c = a, c2 = 9.+a; + int d = 3+b+b2, d2 = d+b2-b; + float e = c*c2, e2 = 4.-c-c2; + int f = 3*d*d2, f2 = 4/d2-d; + float g = e-float(f)+e2, g2 = 4.-g+e2+e; + int h = 3*f-f2, h2 = 7*f2-f; + return float(h*h2)*g*g2*x + g2/float(h2-h); +} \ No newline at end of file diff --git a/tests/unit/reuse-var.frag.expected b/tests/unit/reuse-var.frag.expected new file mode 100644 index 00000000..6bd7ea6b --- /dev/null +++ b/tests/unit/reuse-var.frag.expected @@ -0,0 +1,36 @@ +float simple_var_decl_reuse(float x) +{ + float a=1.+x; + int b=3+int(x); + vec3 sep=vec3(0); + int b2=5+int(a); + float c=a; + sep+=vec3(0); + float c2=9.+a; + int d=3+b+b2; + sep+=vec3(0); + int d2=d+b2-b; + float e=c*c2; + sep+=vec3(0); + float e2=4.-c-c2; + int f=3*d*d2; + sep+=vec3(0); + int f2=4/d2-d; + float g=e-float(f)+e2; + int g2=int(4.-g+e2+e),h=3*f-f2; + sep+=vec3(0); + int h2=7*f2-f; + return length(sep)+float(h*h2)*g*float(g2)*x+float(g2/h2-h); +} +float multidecl_var_decl_reuse(float x) +{ + float a=1.+x; + int b=3+int(x),b2=5+int(a); + float c=a,c2=9.+a; + int d=3+b+b2,d2=d+b2-b; + float e=c*c2,e2=4.-c-c2; + int f=3*d*d2,f2=4/d2-d; + float g=e-float(f)+e2,g2=4.-g+e2+e; + int h=3*f-f2,h2=7*f2-f; + return float(h*h2)*g*g2*x+g2/float(h2-h); +}