Inlining vs inout

[therontarigo]

When inlining a function, Minifier behaves as though all parameters are qualified inout.

Contrived example reproducing the bug:

out vec4 O;
float i_func(float p) {
  return p-=2.,p*p;
}
void main() {
  float x=gl_FragCoord.x;
  O.x=i_func(x);
  O.y=x;
}

out vec4 v;
void main()
{
  float x=gl_FragCoord.x;
  v.x=(x-=2.,x*x);
  v.y=x;
}

With the current single-expression-only inlining, this hasn't been an issue in practice. However it is likely to come up if allowing #470 .
The simplest fix is to enforce a rule:
Every parameter declaration of a function to be inlined must be either qualified inout or used immutably (no assignments).

[eldritchconundrum]

Forcing inlining can easily cause bugs, because it bypasses all the verifications that inlining the function will behave correctly, that are described here:

shader-minifier/src/inlining.fs

Lines 172 to 197 in 3e25bdf

	// To ensure correctness, we verify if it's safe to inline.
	//
	// [A] Only inline a function if it never refers to a global function or variable by a name that is shadowed by a local variable in scope at the call site.
	// [B] Only inline a function if it has only one call site.
	// Exception: if the body is "trivial" it will be inlined at all call sites.
	// [C] Only inline a function if it is a single expression return.
	// This also ensures the function does not declare any locals.
	// [D] Only inline a function if it uses its 'in' parameters at most once,
	// or if the parameter is used multiple times but the argument expression can be duplicated at the call site.
	// No attempt is made to inline in other cases. For example, it would be correct to inline
	// when the parameter is written but the argument is a lvalue that doesn't make any side effect and is not used after the call site.
	// Repeating the expression could increase the shader size or decrease run time performance.
	// [E] Only inline a function if its 'in' parameters are never written to (through assignOps or calling an out or inout function or operator).
	// No attempt is made to find if the passed argument is an lvalue that's never used after calling the function to inline.
	// No attempt is made to copy the argument into a newly declared local variable at the call site to get correct writing semantics.
	// [F] Only inline a function if it has no 'out' or 'inout' parameters.
	// 'out' or 'inout' parameters must be lvalues, which simplifies things. But there are problems to watch out for.
	// Evaluating them could have side effects (e.g. a[b++]), which is a problem if they are used more than once.
	// If the 'out' parameters are read from, inlining can change the behavior.
	// It's fine if 'out' parameters are written in more than one place.
	// [G] Only inline a function if the argument expressions are pure.
	// Inlining can change the evaluation order of the arguments, and will remove unused arguments.
	// This is fine when they are pure. Except in one case:
	// BUG: Function inlining can delay the evaluation order of an argument expression that reads a global,
	// and the global can be modified by the inlined function before it's evaluated as part of the argument.
	// int g = 0; int foo(int a) { return ++g - a; } int main() { return foo(g); } // `foo(g)` is 1, but `g++ - g` would be 0

[therontarigo]

Ah, so perhaps this is user-error as I did not realize i_ overrides this particular verification (I can't think of a legitimate reason for allowing it). Sometimes i_ is necessary on an already safe to inline function for overriding the heuristics to save some bytes and it would be helpful to not forgo all reasonable checks in that situation.

[therontarigo]

In a similar spirit to rule G requiring expression purity for in expressions, it would be reasonable to relax rule F to require that if out or inout is used, the argument at the call site must be a variable reference only (must not be a compound lvalue expression).

[therontarigo]

If the 'out' parameters are read from, inlining can change the behavior

Causing changes to already undefined behavior is acceptable. But it can introduce a bug in the case that the variable used as the out argument is also used in an argument expression.

int func (out int O, int I);

If func reads O before writing it, it's UB - always.

safe: (func(x,y), x+y) (x+y, func(x,y))
prone to driver bugs: (x+func(x,y)) (func(x,y)+x)
inlining can break the behavior: (func(x,x)) (func(x,x+y))

[eldritchconundrum]

Ah, so perhaps this is user-error as I did not realize i_ overrides this particular verification (I can't think of a legitimate reason for allowing it). Sometimes i_ is necessary on an already safe to inline function for overriding the heuristics to save some bytes and it would be helpful to not forgo all reasonable checks in that situation.

Yes, that's how I see it: detecting if it's safe or not to inline isn't easy, i_ is given as an advanced (and dangerous) tool for overriding the minifier's analysis, so currently it's the user's fault for not making sure that overriding the minifier's analysis is safe. Maybe we could refine this (to selectively override only some rules?) but that seems complex for little benefits.

Improving the current rules is possible but it can be tricky to get the details right. They were chosen to balance multiple goals: preventing inlining from breaking the code, but still covering a lot of useful cases, but also not making the implementation too complex and the analysis too slow.

[therontarigo]

Would it make sense to have a way to override inlining heuristic (inlining hint) that's separate from the existing "force inline" feature?