From a543e802a4651da27e1bb06e54052a73bd267c53 Mon Sep 17 00:00:00 2001 From: Eldritch Conundrum Date: Sun, 26 May 2024 15:15:29 -0700 Subject: [PATCH] Fixes #317, inlining bugs (#409) * Stop removing side-effecting init of unused locals. * Stop removing unassigned-but-used locals. This is accomplished by moving the unassigned removal from the "simple" inlining to the "safe" inlining. --- src/analyzer.fs | 49 ++++++++++++++----- src/rewriter.fs | 34 +++---------- tests/compression_results.log | 4 +- ...real_party_is_in_your_pocket.frag.expected | 49 ++++++++++--------- tests/unit/inline.aggro.expected | 5 ++ tests/unit/inline.expected | 5 ++ tests/unit/inline.frag | 2 - tests/unit/inline.no.expected | 5 ++ tests/unit/unused_removal.frag | 15 +++++- tests/unit/unused_removal.frag.expected | 9 +++- 10 files changed, 108 insertions(+), 69 deletions(-) diff --git a/src/analyzer.fs b/src/analyzer.fs index 7406d8eb..8bb73970 100644 --- a/src/analyzer.fs +++ b/src/analyzer.fs @@ -8,6 +8,28 @@ open Options.Globals // information in the AST nodes, e.g. find which variables are modified, // which declarations can be inlined. +let rec sideEffects = function + | Var _ -> [] + | Int _ + | Float _ -> [] + | Dot(v, _) -> sideEffects v + | Subscript(e1, e2) -> (e1 :: (Option.toList e2)) |> List.collect sideEffects + | FunCall(Var fct, args) when Builtin.pureBuiltinFunctions.Contains(fct.Name) -> args |> List.collect sideEffects + | FunCall(Var fct, args) as e -> + match fct.Declaration with + | Declaration.UserFunction fd when not fd.hasExternallyVisibleSideEffects -> args |> List.collect sideEffects + | _ -> [e] + | FunCall(Op "?:", [condExpr; thenExpr; elseExpr]) as e -> + if sideEffects thenExpr = [] && sideEffects elseExpr = [] + then sideEffects condExpr + else [e] // We could apply sideEffects to thenExpr and elseExpr, but the result wouldn't necessarily have the same type... + | FunCall(Op op, args) when not(Builtin.assignOps.Contains(op)) -> args |> List.collect sideEffects + | FunCall(Dot(_, field) as e, args) when field = "length" -> (e :: args) |> List.collect sideEffects + | FunCall(Subscript _ as e, args) -> (e :: args) |> List.collect sideEffects + | e -> [e] + +let rec isPure e = sideEffects e = [] + let varUsesInStmt stmt = let mutable idents = [] let collectLocalUses _ = function @@ -57,7 +79,8 @@ module private VariableInlining = for def in declElts do // can only inline if it has a value match def.init with - | None -> () + | None -> + localDefs.[def.name.Name] <- (def.name, true) | Some init -> localExpr <- init :: localExpr let isConst = // INCORRECT: the shadowing variables are merged! they might hide a writing reference! @@ -73,13 +96,18 @@ module private VariableInlining = for def in localDefs do let ident, isConst = def.Value if not ident.DoNotInline && not ident.ToBeInlined && not ident.VarDecl.Value.isEverWrittenAfterDecl then + let decl = ident.VarDecl.Value.decl match localReferences.TryGetValue(def.Key), allReferences.TryGetValue(def.Key) with - | (_, 1), (_, 1) when isConst -> + | (_, 1), (_, 1) when isConst && decl.init <> None -> debug $"{ident.Loc}: inlining local variable '{Printer.debugIdent ident}' because it's safe to inline (const) and used only once" ident.ToBeInlined <- true | (_, 0), (_, 0) -> - debug $"{ident.Loc}: inlining (removing) local variable '{Printer.debugIdent ident}' because it's safe to inline and unused" - ident.ToBeInlined <- true + let ok = match decl.init with + | Some init -> isPure init + | None -> true + if ok then + debug $"{ident.Loc}: inlining (removing) local variable '{Printer.debugDecl decl}' because it's unused and the init is pure or missing" + ident.ToBeInlined <- true | _ -> () let isTrivialExpr = function @@ -93,11 +121,11 @@ module private VariableInlining = match init with | e when isTrivialExpr e -> true | _ when not options.aggroInlining -> false - // Allow a few things to be inlined with aggroInlining + // Allow a few things to be inlined with aggroInlining (even if they have side effects!) | Var v | Dot (Var v, _) -> - match v.Declaration with // Don't inline the use of a variable that's already marked for inlining! - | Declaration.Variable vd when not vd.decl.name.ToBeInlined -> isEffectivelyConst v + match v.VarDecl with // Don't inline the use of a variable that's already marked for inlining! + | Some vd when not vd.decl.name.ToBeInlined -> isEffectivelyConst v | _ -> false | FunCall(Op op, args) -> not (Builtin.assignOps.Contains op) && @@ -120,12 +148,9 @@ module private VariableInlining = not def.name.DoNotInline && not def.name.VarDecl.Value.isEverWrittenAfterDecl then match def.init with - | None -> + | None -> () // Top-level values are special, in particular in HLSL. Keep them for now. - if level <> Level.TopLevel then - // Never-written locals without init should be unused: inline (remove) them. - debug $"{def.name.Loc}: inlining (removing) unassigned local '{Printer.debugDecl def}'" - def.name.ToBeInlined <- true + // Never-written locals without init might be unused, but we don't know for sure here. Let safe inlining handle them. | Some init -> if isSimpleEnoughToInline init then // Never-written locals and globals are inlined when their value is "simple enough". diff --git a/src/rewriter.fs b/src/rewriter.fs index 7fc97ad9..539a309a 100644 --- a/src/rewriter.fs +++ b/src/rewriter.fs @@ -13,28 +13,6 @@ let renameField field = let private commaSeparatedExprs = List.reduce (fun a b -> FunCall(Op ",", [a; b])) -let rec private sideEffects = function - | Var _ -> [] - | Int _ - | Float _ -> [] - | Dot(v, _) -> sideEffects v - | Subscript(e1, e2) -> (e1 :: (Option.toList e2)) |> List.collect sideEffects - | FunCall(Var fct, args) when Builtin.pureBuiltinFunctions.Contains(fct.Name) -> args |> List.collect sideEffects - | FunCall(Var fct, args) as e -> - match fct.Declaration with - | Declaration.UserFunction fd when not fd.hasExternallyVisibleSideEffects -> args |> List.collect sideEffects - | _ -> [e] - | FunCall(Op "?:", [condExpr; thenExpr; elseExpr]) as e -> - if sideEffects thenExpr = [] && sideEffects elseExpr = [] - then sideEffects condExpr - else [e] // We could apply sideEffects to thenExpr and elseExpr, but the result wouldn't necessarily have the same type... - | FunCall(Op op, args) when not(Builtin.assignOps.Contains(op)) -> args |> List.collect sideEffects - | FunCall(Dot(_, field) as e, args) when field = "length" -> (e :: args) |> List.collect sideEffects - | FunCall(Subscript _ as e, args) -> (e :: args) |> List.collect sideEffects - | e -> [e] - -let rec private isPure e = sideEffects e = [] - module private RewriterImpl = // Remove useless spaces in macros @@ -202,7 +180,8 @@ module private RewriterImpl = // Swap operands to get rid of parentheses // x*(y*z) -> y*z*x - | FunCall(Op "*", [NoParen x; FunCall(Op "*", [y; z])]) when isPure x && isPure y && isPure z -> + | FunCall(Op "*", [NoParen x; FunCall(Op "*", [y; z])]) + when Analyzer.isPure x && Analyzer.isPure y && Analyzer.isPure z -> FunCall(Op "*", [FunCall(Op "*", [y; z]); x]) |> env.fExpr env // x+(y+z) -> x+y+z // x+(y-z) -> x+y-z @@ -344,6 +323,7 @@ module private RewriterImpl = | Dot(e, field) when options.canonicalFieldNames <> "" -> Dot(e, renameField field) | Var s as e -> + // Replace uses of inlined variables. match env.vars.TryFind s.Name with | Some (_, {name = id; init = Some init}) when id.ToBeInlined -> didInline.Value <- true @@ -576,7 +556,7 @@ module private RewriterImpl = // Remove unused assignment immediately followed by re-assignment: m=14.;m=58.; -> 14.;m=58.; | 0 -> Some [Expr init1; assign2] // Transform is safe even if the var is an out parameter. // Inline this single use of a used-once assignment into the immediately following re-assignment: m=14.;m=58.-m; -> m=58.-14.; - | 1 when isPure init1 && isPure init2 -> // This is ok only if init1 is pure and the part of init2 before using m is pure. + | 1 when Analyzer.isPure init1 && Analyzer.isPure init2 -> // This is ok only if init1 is pure and the part of init2 before using m is pure. let newInit2 = replaceUsesOfIdentByExpr init2 name.Name init1 debug $"{name.Loc}: merge consecutive pure assignments to the same local '{name}'" Some [Expr (FunCall (Op "=", [Var name2; newInit2]))] @@ -587,12 +567,12 @@ module private RewriterImpl = when declElt.name.Name = name2.Name -> match countUsesOfIdentName init2 declElt.name.Name with | 0 -> - match declElt.init |> Option.map sideEffects |> Option.defaultValue [] with + match declElt.init |> Option.map Analyzer.sideEffects |> Option.defaultValue [] with // float m=14;m=58.; -> float m=58.; | [] -> Some [Decl (ty, [{declElt with init = Some init2}])] // float m=f();m=58.; -> float m=(f(),58.); | es -> Some [Decl (ty, [{declElt with init = Some (commaSeparatedExprs (es @ [init2]))}])] - | 1 when Option.forall isPure declElt.init && isPure init2 -> + | 1 when Option.forall Analyzer.isPure declElt.init && Analyzer.isPure init2 -> let newInit = match declElt.init with | None -> init2 // float m; m=1.; -> float m=1.; @@ -606,7 +586,7 @@ module private RewriterImpl = // Reduces impure expression statements to their side effects. let b = b |> List.collect (function | Expr e -> - match sideEffects e with + match Analyzer.sideEffects e with | [] -> [] // Remove pure statements. | [e] -> [Expr e] | sideEffects -> [Expr (commaSeparatedExprs sideEffects)] diff --git a/tests/compression_results.log b/tests/compression_results.log index 50d3a79d..13b7a945 100644 --- a/tests/compression_results.log +++ b/tests/compression_results.log @@ -18,7 +18,7 @@ oscars_chair.frag 4648 => 986.069 robin.frag 6199 => 1039.306 slisesix.frag 4497 => 890.639 terrarium.frag 3575 => 747.116 -the_real_party_is_in_your_pocket.frag 11986 => 1774.550 +the_real_party_is_in_your_pocket.frag 11988 => 1775.087 valley_ball.glsl 4307 => 881.820 yx_long_way_from_home.frag 2936 => 598.845 -Total: 133457 => 23303.455 +Total: 133459 => 23303.992 diff --git a/tests/real/the_real_party_is_in_your_pocket.frag.expected b/tests/real/the_real_party_is_in_your_pocket.frag.expected index 826bd915..c139aa43 100644 --- a/tests/real/the_real_party_is_in_your_pocket.frag.expected +++ b/tests/real/the_real_party_is_in_your_pocket.frag.expected @@ -178,12 +178,13 @@ const char *the_real_party_is_in_your_pocket_frag = "vec4 h(vec3 l,vec3 v,out vec3 y)" "{" "float m=10.;" - "vec3 a;" - "vec4 i=vec4(-1);" + "vec2 a;" + "vec3 i;" + "vec4 f=vec4(-1);" "{" "float x=m;" - "vec3 f=l-vec3(0,0,.2),z=v,d=vec3(1.8,.98,.4);" - "vec2 w=n(f-vec3(0,0,.1),z,-d,d);" + "vec3 z=l-vec3(0,0,.2),p=v,d=vec3(1.8,.98,.4);" + "vec2 w=n(z-vec3(0,0,.1),p,-d,d);" "float r=-1.,c;" "vec4 k;" "if(w.x0.&&w.xw.y||abs(c)<1e-5)" "break;" @@ -199,27 +200,27 @@ const char *the_real_party_is_in_your_pocket_frag = "}" "if(r0.)" - "return y=a,i;" + "if(f.x>0.)" + "return y=i,f;" "m=-1.;" - "vec2 x=vec2(-1,-3);" - "float f=(x.x-l.y)/v.y,z=(x.y-l.z)/v.z;" - "vec2 w=h((l-vec3(0,x+2.))*vec3(0,.5,.5),v*vec3(0,.5,.5));" - "x=l.yz+v.yz*w.y-x-2.;" - "if(w.x0.&&x.x<0.&&x.y<0.)" - "y=vec3(-x*.5,0).zxy,m=w.y;" - "else if(f>0.&&(f0.&&(z0.&&a.x<0.&&a.y<0.)" + "y=vec3(-a*.5,0).zxy,m=w.y;" + "else if(x>0.&&(x0.&&(z5.||abs(a.y)>2.5||abs(a.z)>3.3?" + "i=l+v*m;" + "return abs(i.x)>5.||abs(i.y)>2.5||abs(i.z)>3.3?" "vec4(-1):" "vec4(m,0,0,0);" "}" @@ -325,14 +326,14 @@ const char *the_real_party_is_in_your_pocket_frag = "}" "}" "{" - "vec2 l=vec2(160,144)/1.5,v=(f.zw-vec2(-.001,.486))*3.75*vec2(l.y/l.x,1)*.5+.5,x=fract(v*l);" + "vec2 l=vec2(160,144)/1.5,v=(f.zw-vec2(-.001,.486))*3.75*vec2(l.y/l.x,1)*.5+.5,z=fract(v*l);" "v=floor(v*l)/l;" "if(max(abs(v.x-.5),abs(v.y-.5))<.5)" "{" "vec3 i=vec3(0);" "vec2 m=(v*2.-1.)*vec2(l.x/l.y,1);" - "float f=floor(v.y*8.);" - "i=mix(.5+.5*cos(vec3(.8,.3,2)*(f+1.)),vec3(1),step(min(min(min(min(min(min(min(min(min(min(min(min(min(min(min(min(min(n(vec4(1.54,.53,.91,.72),m),n(vec4(.63,.78,.91,.72),m)),n(vec4(.61,1.675,.72,.64),m)),n(vec4(2.7,3.11,.72,.64),m)),n(vec4(3.45,3.65,.72,.64),m)),n(vec4(4.71,5.02,.72,.64),m)),n(vec4(5.3,5.51,.72,.64),m)),n(vec4(5.96,6.43,.72,.64),m)),n(vec4(3.2,1.27,.45,.35),m)),n(vec4(1.3,2.3,.45,.35),m)),n(vec4(2.58,4.2,.45,.35),m)),n(vec4(3.2,3.95,.35,.25),m)),n(vec4(5.2,5.93,.35,.25),m)),n(vec4(7.9,8.15,.35,.25),m)),n(vec4(.2,1.16,.32,.17),m)),max(length(m)-.84,-length(m)+.72)),max(length(m)-.52,-length(m)+.45)),max(length(m)-.17,-length(m)+.08)),0.))*vec3(1,1,.8)*(smoothstep(.1,.2,x.x)*smoothstep(.1,.2,x.y));" + "float x=floor(v.y*8.);" + "i=mix(.5+.5*cos(vec3(.8,.3,2)*(x+1.)),vec3(1),step(min(min(min(min(min(min(min(min(min(min(min(min(min(min(min(min(min(n(vec4(1.54,.53,.91,.72),m),n(vec4(.63,.78,.91,.72),m)),n(vec4(.61,1.675,.72,.64),m)),n(vec4(2.7,3.11,.72,.64),m)),n(vec4(3.45,3.65,.72,.64),m)),n(vec4(4.71,5.02,.72,.64),m)),n(vec4(5.3,5.51,.72,.64),m)),n(vec4(5.96,6.43,.72,.64),m)),n(vec4(3.2,1.27,.45,.35),m)),n(vec4(1.3,2.3,.45,.35),m)),n(vec4(2.58,4.2,.45,.35),m)),n(vec4(3.2,3.95,.35,.25),m)),n(vec4(5.2,5.93,.35,.25),m)),n(vec4(7.9,8.15,.35,.25),m)),n(vec4(.2,1.16,.32,.17),m)),max(length(m)-.84,-length(m)+.72)),max(length(m)-.52,-length(m)+.45)),max(length(m)-.17,-length(m)+.08)),0.))*vec3(1,1,.8)*(smoothstep(.1,.2,z.x)*smoothstep(.1,.2,z.y));" "y+=a*3.*i;" "r=vec3(.1);" "}" diff --git a/tests/unit/inline.aggro.expected b/tests/unit/inline.aggro.expected index c0d453f2..428268fb 100644 --- a/tests/unit/inline.aggro.expected +++ b/tests/unit/inline.aggro.expected @@ -90,6 +90,11 @@ float inlineWithShadowing(float x) } return sin(2.5); } +float inline_uninitialized() +{ + float c; + return c; +} float glo; float noinline_readsTheGlobal() { diff --git a/tests/unit/inline.expected b/tests/unit/inline.expected index 50c92992..ebac389d 100644 --- a/tests/unit/inline.expected +++ b/tests/unit/inline.expected @@ -91,6 +91,11 @@ float inlineWithShadowing(float x) } return inl; } +float inline_uninitialized() +{ + float c; + return c; +} float glo; float noinline_readsTheGlobal() { diff --git a/tests/unit/inline.frag b/tests/unit/inline.frag index 16d1baa5..91226f16 100644 --- a/tests/unit/inline.frag +++ b/tests/unit/inline.frag @@ -141,7 +141,6 @@ float inlineWithShadowing(float x) { return inl; } -/* // repro for a bug float inline_uninitialized() { @@ -150,7 +149,6 @@ float inline_uninitialized() float c; return c; } -*/ // repro for a bug float glo; diff --git a/tests/unit/inline.no.expected b/tests/unit/inline.no.expected index 5691ee78..b0fdd0a3 100644 --- a/tests/unit/inline.no.expected +++ b/tests/unit/inline.no.expected @@ -105,6 +105,11 @@ float inlineWithShadowing(float x) } return inl; } +float inline_uninitialized() +{ + float c; + return c; +} float glo; float noinline_readsTheGlobal() { diff --git a/tests/unit/unused_removal.frag b/tests/unit/unused_removal.frag index 2c866d40..c6390b04 100644 --- a/tests/unit/unused_removal.frag +++ b/tests/unit/unused_removal.frag @@ -26,10 +26,23 @@ float foo(float hidden) { return hidden+noinlinevar; } +float glob; +float side_effect() { + return glob++; +} + +vec4 noinline_test() +{ + vec4 bb; + return bb; +} + void main(){ + float unused_var = side_effect(); + if (false) { noinline_actually_unreachable2(); return; } - gl_FragColor = vec4(foo(3.), g()); + gl_FragColor = vec4(foo(3.), g()) + noinline_test(); } diff --git a/tests/unit/unused_removal.frag.expected b/tests/unit/unused_removal.frag.expected index e2d114cc..a02e6a6d 100644 --- a/tests/unit/unused_removal.frag.expected +++ b/tests/unit/unused_removal.frag.expected @@ -1,11 +1,18 @@ vec2 pos=vec2(.5); +float glob; float foo() { float noinlinevar=0.; noinlinevar++; return 3.+noinlinevar; } +vec4 noinline_test() +{ + vec4 bb; + return bb; +} void main() { - gl_FragColor=vec4(foo(),vec3(0)); + float unused_var=glob++; + gl_FragColor=vec4(foo(),vec3(0))+noinline_test(); }