Fixes #388, aggressive inlining bug (#408)

src/analyzer.fs

@@ -88,14 +88,17 @@ module private VariableInlining =
         | Float _ -> true
         | _ -> false
 
-    // Detect if a variable can be inlined, based on its value.
-    let canBeInlined (init: Expr) =
+    // Detect if a variable can be inlined in multiple places, based on its value.
+    let isSimpleEnoughToInline (init: Expr) =
         match init with
         | e when isTrivialExpr e -> true
         | _ when not options.aggroInlining -> false
         // Allow a few things to be inlined with aggroInlining
         | Var v
-        | Dot (Var v, _) -> isEffectivelyConst 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
+            | _ -> false
         | FunCall(Op op, args) ->
             not (Builtin.assignOps.Contains op) &&
                 args |> List.forall isTrivialExpr
@@ -124,20 +127,28 @@ module private VariableInlining =
                             debug $"{def.name.Loc}: inlining (removing) unassigned local '{Printer.debugDecl def}'"
                             def.name.ToBeInlined <- true
                     | Some init ->
-                        if canBeInlined init then
+                        if isSimpleEnoughToInline init then
                             // Never-written locals and globals are inlined when their value is "simple enough".
                             // This can increase non-compressed size but decreases compressed size.
                             let varKind = match level with Level.TopLevel -> "global" | Level.InFunc -> "local"
                             debug $"{def.name.Loc}: inlining {varKind} variable '{Printer.debugDecl def}' because it's never written and has a 'simple' definition"
                             def.name.ToBeInlined <- true
         | _ -> ()
 
-    let markInlinableVariables li =
+    let markSafelyInlinableVariables li =
+        let mapStmt _ stmt =
+            match stmt with
+            | Block b -> markSafelyInlinableLocals b
+            | _ -> ()
+            stmt
+        mapTopLevel (mapEnv (fun _ -> id) mapStmt) li |> ignore<TopLevel list>
+        ()
+
+    let markSimpleInlinableVariables li =
         let mapStmt _ stmt =
             match stmt with
             | Decl d -> markUnwrittenVariablesWithSimpleInit Level.InFunc d
             | ForD (d, _, _, _) -> markUnwrittenVariablesWithSimpleInit Level.InFunc d
-            | Block b -> markSafelyInlinableLocals b
             | _ -> ()
             stmt
         // Visit locals
@@ -149,7 +160,10 @@ module private VariableInlining =
             | _ -> ()
         ()
 
-let markInlinableVariables = VariableInlining.markInlinableVariables
+let markInlinableVariables li =
+    VariableInlining.markSafelyInlinableVariables li
+    // "simple" inlining must come after "safe" inlining, because it must check that it's not going to inline a var already being inlined.
+    VariableInlining.markSimpleInlinableVariables li
 
 let markWrites topLevel = // calculates hasExternallyVisibleSideEffects, for inlining
     let findWrites (env: MapEnv) = function

tests/real/orchard.frag.expected

@@ -269,8 +269,8 @@ void mainImage(out vec4 fragColour,vec2 fragCoord)
 
 #endif
 
-  vec3 col=vec3(0),sky=mix(vec3(FOG_COLOUR),vec3(0,.4,.6),abs((viewMat(uv.y+sin(time*.5)*.4,uv.x+time*.5)*normalize(vec3(0,0,1))).y));
-  float alpha=marchScene(camera,viewMat(uv.y+sin(time*.5)*.4,uv.x+time*.5)*normalize(vec3(0,0,1)),fragCoord);
+  vec3 rd=viewMat(uv.y+sin(time*.5)*.4,uv.x+time*.5)*normalize(vec3(0,0,1)),col=vec3(0),sky=mix(vec3(FOG_COLOUR),vec3(0,.4,.6),abs(rd.y));
+  float alpha=marchScene(camera,rd,fragCoord);
   vec4 mat;
   if(alpha>0.)
     for(int i=0;i<spointer;i++)
@@ -287,18 +287,18 @@ void mainImage(out vec4 fragColour,vec2 fragCoord)
               leavesTexture(pos,nor):
               fruitTexture(pos,nor,float(matID-3));
         mat*=randomTint(pos);
-        vec3 temp=lighting(mat,pos,nor,viewMat(uv.y+sin(time*.5)*.4,uv.x+time*.5)*normalize(vec3(0,0,1)),d);
+        vec3 temp=lighting(mat,pos,nor,rd,d);
         if(matID==3)
           temp=temp*.4+vec3(.15,.01,0);
         temp=mix(sky,temp,exp(-d*.01));
         col+=temp*stack[i].alpha;
       }
-  vec4 cc=getClouds(camera,viewMat(uv.y+sin(time*.5)*.4,uv.x+time*.5)*normalize(vec3(0,0,1)));
-  sky=mix(sky+pow(max(dot(sunLight,viewMat(uv.y+sin(time*.5)*.4,uv.x+time*.5)*normalize(vec3(0,0,1))),0.),20.)*SUN_COLOUR*.03,cc.xyz,cc.w);
+  vec4 cc=getClouds(camera,rd);
+  sky=mix(sky+pow(max(dot(sunLight,rd),0.),20.)*SUN_COLOUR*.03,cc.xyz,cc.w);
   col+=sky*(1.-alpha);
   float d=stack[0].dist;
-  col+=floatingSeeds(camera,viewMat(uv.y+sin(time*.5)*.4,uv.x+time*.5)*normalize(vec3(0,0,1)),d);
-  col=clamp(col+pow(max(dot(sunLight,viewMat(uv.y+sin(time*.5)*.4,uv.x+time*.5)*normalize(vec3(0,0,1))),0.),6.)*SUN_COLOUR*.2,0.,1.);
+  col+=floatingSeeds(camera,rd,d);
+  col=clamp(col+pow(max(dot(sunLight,rd),0.),6.)*SUN_COLOUR*.2,0.,1.);
   col=col*col*(3.-2.*col);
   vec2 q=fragCoord/iResolution.xy;
   col=col*(.3+.7*pow(90.*q.x*q.y*(1.-q.x)*(1.-q.y),.5))*smoothstep(0.,5.,time);

tests/unit/inline.aggro.expected

@@ -101,3 +101,12 @@ float dontCompressAssigments()
   glo=50.+noinline_readsTheGlobal();
   return glo*glo;
 }
+vec3 repro(vec2 fragCoord,float iTime)
+{
+  iTime+=(fragCoord.xy/(fragCoord*2.).xy).x*10.;
+  vec3 used_many_times=vec3((fragCoord.y+sin(iTime*.5)*.4-fragCoord.x+iTime*.5)*normalize(vec3(0,0,1))),col=used_many_times,sky=col+used_many_times;
+  vec4 cc=vec4(used_many_times-used_many_times,1);
+  sky+=pow(max(dot(sky,used_many_times),0.),20.)*.03;
+  col=vec3(dot(col+mix(sky,cc.xyz,cc.w)+used_many_times,used_many_times));
+  return col+pow(max(dot(sky,used_many_times),0.),6.)*.2;
+}

tests/unit/inline.expected

@@ -102,3 +102,12 @@ float dontCompressAssigments()
   glo=50.+noinline_readsTheGlobal();
   return glo*glo;
 }
+vec3 repro(vec2 fragCoord,float iTime)
+{
+  iTime+=(fragCoord.xy/(fragCoord*2.).xy).x*10.;
+  vec3 used_many_times=vec3((fragCoord.y+sin(iTime*.5)*.4-fragCoord.x+iTime*.5)*normalize(vec3(0,0,1))),col=used_many_times,sky=col+used_many_times,camera=used_many_times;
+  vec4 cc=vec4(camera-used_many_times,1);
+  sky+=pow(max(dot(sky,used_many_times),0.),20.)*.03;
+  col=vec3(dot(col+mix(sky,cc.xyz,cc.w)+camera,used_many_times));
+  return col+pow(max(dot(sky,used_many_times),0.),6.)*.2;
+}

tests/unit/inline.frag

@@ -164,3 +164,20 @@ float dontCompressAssigments()
 	glo = 50. + noinline_readsTheGlobal();
 	return glo*glo;
 }
+
+// repro for dual-kind-mixing aggressive-inlining bug
+vec3 repro(vec2 fragCoord, float iTime)
+{
+    float time = iTime*1.+(fragCoord.xy / (fragCoord*2.).xy).x*10.0;
+
+    vec3 long_and_used_only_once = vec3((fragCoord.y+sin(time*.5)*.4 - fragCoord.x+time*.5)*normalize(vec3(0,0, 1.)));
+    vec3 used_many_times = long_and_used_only_once;
+
+    vec3 col = used_many_times;
+    vec3 sky  = col + used_many_times;
+    vec3 camera = used_many_times;
+    vec4 cc = vec4(camera - used_many_times, 1.);
+    sky+= pow(max(dot(sky, used_many_times), 0.0), 20.0) * .03;
+    col = vec3(dot(col + mix(sky, cc.xyz, cc.w) + camera, used_many_times));
+    return col + pow(max(dot(sky, used_many_times), 0.0), 6.0) * .2;
+}

tests/unit/inline.no.expected

@@ -116,3 +116,13 @@ float dontCompressAssigments()
   glo=50.+noinline_readsTheGlobal();
   return glo*glo;
 }
+vec3 repro(vec2 fragCoord,float iTime)
+{
+  iTime+=(fragCoord.xy/(fragCoord*2.).xy).x*10.;
+  vec3 long_and_used_only_once=vec3((fragCoord.y+sin(iTime*.5)*.4-fragCoord.x+iTime*.5)*normalize(vec3(0,0,1))),used_many_times=long_and_used_only_once,col=used_many_times,sky=col+used_many_times;
+  long_and_used_only_once=used_many_times;
+  vec4 cc=vec4(long_and_used_only_once-used_many_times,1);
+  sky+=pow(max(dot(sky,used_many_times),0.),20.)*.03;
+  col=vec3(dot(col+mix(sky,cc.xyz,cc.w)+long_and_used_only_once,used_many_times));
+  return col+pow(max(dot(sky,used_many_times),0.),6.)*.2;
+}