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.
laurentlb · May 26, 2024 · a543e80 · a543e80
1 parent e799bee
commit a543e80
Show file tree

Hide file tree

Showing 10 changed files with 108 additions and 69 deletions.
diff --git 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
@@ -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
@@ -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
@@ -178,48 +178,49 @@ 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.x<w.y&&w.y>0.&&w.x<x)"
        "{"
          "r=max(0.,w.x);"
          "for(int y=0;y<60;++y)"
            "{"
-             "k=s(f+z*r);"
+             "k=s(z+p*r);"
              "c=k.x;"
              "if(r>w.y||abs(c)<1e-5)"
                "break;"
              "r+=c;"
            "}"
          "if(r<w.y||r<0.)"
            "{"
-             "vec3 l=vec3(.002,0,0),m=f+z*r;"
-             "a=normalize(vec3(s(m+l.xyy).x,s(m+l.yxy).x,s(m+l.yyx))-c);"
-             "i=vec4(r,k.yzw);"
+             "vec3 l=vec3(.002,0,0),m=z+p*r;"
+             "i=normalize(vec3(s(m+l.xyy).x,s(m+l.yxy).x,s(m+l.yyx))-c);"
+             "f=vec4(r,k.yzw);"
            "}"
        "}"
    "}"
-   "if(i.x>0.)"
-     "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.x<w.y&&w.y>0.&&x.x<0.&&x.y<0.)"
-     "y=vec3(-x*.5,0).zxy,m=w.y;"
-   "else if(f>0.&&(f<z||z<0.))"
-     "y=vec3(0,1,0),m=f;"
-   "else if(z>0.&&(z<f||f<0.))"
+   "a=vec2(-1,-3);"
+   "float x=(a.x-l.y)/v.y,z=(a.y-l.z)/v.z;"
+   "vec2 w=h((l-vec3(0,a+2.))*vec3(0,.5,.5),v*vec3(0,.5,.5));"
+   "a=l.yz+v.yz*w.y-a-2.;"
+   "if(w.x<w.y&&w.y>0.&&a.x<0.&&a.y<0.)"
+     "y=vec3(-a*.5,0).zxy,m=w.y;"
+   "else if(x>0.&&(x<z||z<0.))"
+     "y=vec3(0,1,0),m=x;"
+   "else if(z>0.&&(z<x||x<0.))"
      "y=vec3(0,0,1),m=z;"
-   "a=l+v*m;"
-   "return abs(a.x)>5.||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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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();
 }