From: Mikko Rasa <tdb@tdb.fi>
Date: Wed, 10 Mar 2021 11:01:12 +0000 (+0200)
Subject: Rewrite UnusedVariableRemover
X-Git-Url: http://git.tdb.fi/?a=commitdiff_plain;h=d230392;p=libs%2Fgl.git

Rewrite UnusedVariableRemover

The old implementation was getting too complex with its multitude of
flags.  The new one is based on the idea of reaching definition analysis
and is much more straightforward.
---

diff --git a/source/glsl/optimize.cpp b/source/glsl/optimize.cpp
index 87a57eb7..080bfd7c 100644
--- a/source/glsl/optimize.cpp
+++ b/source/glsl/optimize.cpp
@@ -624,15 +624,6 @@ void UnusedTypeRemover::visit(FunctionDeclaration &func)
 }
 
 
-UnusedVariableRemover::VariableInfo::VariableInfo():
-	local(false),
-	output(false),
-	conditionally_assigned(false),
-	referenced(false),
-	interface_block(0)
-{ }
-
-
 UnusedVariableRemover::UnusedVariableRemover():
 	stage(0),
 	interface_block(0),
@@ -644,73 +635,61 @@ UnusedVariableRemover::UnusedVariableRemover():
 bool UnusedVariableRemover::apply(Stage &s)
 {
 	stage = &s;
-	variables.push_back(BlockVariableMap());
 	s.content.visit(*this);
 
-	BlockVariableMap &global_variables = variables.back();
-	set<InterfaceBlock *> used_interface_blocks;
-	Statement *prev_decl = 0;
-	bool output;
-	for(BlockVariableMap::iterator i=global_variables.begin(); i!=global_variables.end(); ++i)
+	for(list<AssignmentInfo>::const_iterator i=assignments.begin(); i!=assignments.end(); ++i)
+		if(i->used_by.empty())
+			unused_nodes.insert(i->node);
+
+	for(map<string, InterfaceBlock *>::const_iterator i=s.interface_blocks.begin(); i!=s.interface_blocks.end(); ++i)
+		if(i->second->instance_name.empty())
+			unused_nodes.insert(i->second);
+
+	for(BlockVariableMap::const_iterator i=variables.begin(); i!=variables.end(); ++i)
 	{
-		if(i->first.declaration!=prev_decl)
-		{
-			prev_decl = i->first.declaration;
-			output = i->second.output;
-		}
-		if(output)
+		if(i->second.output)
 		{
-			if((i->second.referenced || !i->second.assignments.empty()) && i->second.interface_block)
-				used_interface_blocks.insert(i->second.interface_block);
-			continue;
+			/* The last visible assignments of output variables are used by the
+			next stage or the API. */
+			for(vector<AssignmentInfo *>::const_iterator j=i->second.assignments.begin(); j!=i->second.assignments.end(); ++j)
+				unused_nodes.erase((*j)->node);
 		}
 
-		// Mark other unreferenced global variables as unused.
-		if(!i->second.referenced)
+		if(!i->second.output && !i->second.referenced)
 		{
-			if(!i->second.interface_block && !i->first.chain_len)
-				unused_nodes.insert(i->first.declaration);
-			clear_assignments(i->second, true);
+			// Don't remove variables from inside interface blocks.
+			if(!i->second.interface_block)
+				unused_nodes.insert(i->first);
 		}
 		else if(i->second.interface_block)
-			used_interface_blocks.insert(i->second.interface_block);
+			// Interface blocks are kept if even one member is used.
+			unused_nodes.erase(i->second.interface_block);
 	}
-	variables.pop_back();
-
-	for(map<string, InterfaceBlock *>::const_iterator i=s.interface_blocks.begin(); i!=s.interface_blocks.end(); ++i)
-		if(i->second->instance_name.empty() && !used_interface_blocks.count(i->second))
-			unused_nodes.insert(i->second);
 
 	NodeRemover().apply(s, unused_nodes);
 
 	return !unused_nodes.empty();
 }
 
-void UnusedVariableRemover::reference_used(Statement &declaration)
+void UnusedVariableRemover::referenced(const Assignment::Target &target, Node &node)
 {
-	BlockVariableMap &block_vars = variables.back();
-	/* Previous assignments of all subfields of this variable are used by
-	this reference. */
-	for(BlockVariableMap::iterator i=block_vars.lower_bound(&declaration); (i!=block_vars.end() && i->first.declaration==&declaration); ++i)
+	VariableInfo &var_info = variables[target.declaration];
+	var_info.referenced = true;
+	if(!assignment_target)
 	{
-		clear_assignments(i->second, false);
-		i->second.referenced = true;
+		for(vector<AssignmentInfo *>::const_iterator i=var_info.assignments.begin(); i!=var_info.assignments.end(); ++i)
+			(*i)->used_by.push_back(&node);
 	}
-
-	// Always record a reference to the primary declaration, even if it didn't exist before
-	block_vars[&declaration].referenced = true;
 }
 
 void UnusedVariableRemover::visit(VariableReference &var)
 {
-	if(var.declaration && !assignment_target)
-		reference_used(*var.declaration);
+	referenced(var.declaration, var);
 }
 
 void UnusedVariableRemover::visit(InterfaceBlockReference &iface)
 {
-	if(iface.declaration && !assignment_target)
-		reference_used(*iface.declaration);
+	referenced(iface.declaration, iface);
 }
 
 void UnusedVariableRemover::visit(UnaryExpression &unary)
@@ -735,7 +714,7 @@ void UnusedVariableRemover::visit(BinaryExpression &binary)
 void UnusedVariableRemover::visit(Assignment &assign)
 {
 	{
-		SetFlag set(assignment_target, !assign.self_referencing);
+		SetFlag set(assignment_target, (assign.oper->token[0]=='='));
 		assign.left->visit(*this);
 	}
 	assign.right->visit(*this);
@@ -751,36 +730,31 @@ void UnusedVariableRemover::visit(FunctionCall &call)
 	r_side_effects = true;
 }
 
-void UnusedVariableRemover::record_assignment(const Assignment::Target &target, Node &node, bool chained)
+void UnusedVariableRemover::record_assignment(const Assignment::Target &target, Node &node)
 {
-	BlockVariableMap &block_vars = variables.back();
-	for(BlockVariableMap::iterator i=block_vars.lower_bound(target); (i!=block_vars.end() && i->first.declaration==target.declaration); ++i)
+	assignments.push_back(AssignmentInfo());
+	AssignmentInfo &assign_info = assignments.back();
+	assign_info.node = &node;
+	assign_info.target = target;
+
+	/* An assignment to the target hides any assignments to the same target or
+	its subfields. */
+	VariableInfo &var_info = variables[target.declaration];
+	for(unsigned i=0; i<var_info.assignments.size(); ++i)
 	{
-		bool subfield = (i->first.chain_len>=target.chain_len);
+		const Assignment::Target &t = var_info.assignments[i]->target;
+
+		bool subfield = (t.chain_len>=target.chain_len);
 		for(unsigned j=0; (subfield && j<target.chain_len); ++j)
-			subfield = (i->first.chain[j]==target.chain[j]);
-		if(!subfield)
-			break;
+			subfield = (t.chain[j]==target.chain[j]);
 
-		/* An assignment to the target causes any previous unreferenced
-		assignments to the same target or its subfields to be unused. */
-		if(!chained)
-			clear_assignments(i->second, true);
+		if(subfield)
+			var_info.assignments.erase(var_info.assignments.begin()+i);
+		else
+			++i;
 	}
 
-	VariableInfo &var_info = variables.back()[target];
-	var_info.assignments.push_back(&node);
-	var_info.conditionally_assigned = false;
-}
-
-void UnusedVariableRemover::clear_assignments(VariableInfo &var_info, bool mark_unused)
-{
-	if(mark_unused)
-	{
-		for(vector<Node *>::iterator i=var_info.assignments.begin(); i!=var_info.assignments.end(); ++i)
-			unused_nodes.insert(*i);
-	}
-	var_info.assignments.clear();
+	var_info.assignments.push_back(&assign_info);
 }
 
 void UnusedVariableRemover::visit(ExpressionStatement &expr)
@@ -789,15 +763,14 @@ void UnusedVariableRemover::visit(ExpressionStatement &expr)
 	r_side_effects = false;
 	TraversingVisitor::visit(expr);
 	if(r_assignment && r_assignment->target.declaration)
-		record_assignment(r_assignment->target, expr, r_assignment->self_referencing);
+		record_assignment(r_assignment->target, expr);
 	if(!r_side_effects)
 		unused_nodes.insert(&expr);
 }
 
 void UnusedVariableRemover::visit(VariableDeclaration &var)
 {
-	VariableInfo &var_info = variables.back()[&var];
-	var_info.local = true;
+	VariableInfo &var_info = variables[&var];
 	var_info.interface_block = interface_block;
 
 	/* Mark variables as output if they're used by the next stage or the
@@ -808,7 +781,10 @@ void UnusedVariableRemover::visit(VariableDeclaration &var)
 		var_info.output = (var.interface=="out" && (stage->type==Stage::FRAGMENT || var.linked_declaration || !var.name.compare(0, 3, "gl_")));
 
 	if(var.init_expression)
-		record_assignment(&var, *var.init_expression, false);
+	{
+		var_info.initialized = true;
+		record_assignment(&var, *var.init_expression);
+	}
 	TraversingVisitor::visit(var);
 }
 
@@ -821,111 +797,80 @@ void UnusedVariableRemover::visit(InterfaceBlock &iface)
 	}
 	else
 	{
-		VariableInfo &var_info = variables.back()[&iface];
-		var_info.local = true;
+		VariableInfo &var_info = variables[&iface];
 		var_info.output = (iface.interface=="out" && (iface.linked_block || !iface.name.compare(0, 3, "gl_")));
 	}
 }
 
 void UnusedVariableRemover::visit(FunctionDeclaration &func)
 {
-	variables.push_back(BlockVariableMap());
+	if(func.body.body.empty())
+		return;
 
-	for(NodeArray<VariableDeclaration>::iterator i=func.parameters.begin(); i!=func.parameters.end(); ++i)
-		(*i)->visit(*this);
-	func.body.visit(*this);
-
-	BlockVariableMap &block_variables = variables.back();
-
-	/* Mark global variables as conditionally assigned so assignments in other
-	functions won't be removed. */
-	for(BlockVariableMap::iterator i=block_variables.begin(); i!=block_variables.end(); ++i)
-		if(!i->second.local)
-			i->second.conditionally_assigned = true;
+	BlockVariableMap saved_vars = variables;
+	// Assignments from other functions should not be visible.
+	for(BlockVariableMap::iterator i=variables.begin(); i!=variables.end(); ++i)
+		i->second.assignments.resize(i->second.initialized);
+	TraversingVisitor::visit(func);
+	swap(variables, saved_vars);
+	merge_variables(saved_vars);
 
 	/* Always treat function parameters as referenced.  Removing unused
 	parameters is not currently supported. */
 	for(NodeArray<VariableDeclaration>::iterator i=func.parameters.begin(); i!=func.parameters.end(); ++i)
-		block_variables[i->get()].referenced = true;
-
-	merge_down_variables();
+	{
+		BlockVariableMap::iterator j = variables.find(i->get());
+		if(j!=variables.end())
+			j->second.referenced = true;
+	}
 }
 
-void UnusedVariableRemover::merge_down_variables()
+void UnusedVariableRemover::merge_variables(const BlockVariableMap &other_vars)
 {
-	BlockVariableMap &parent_variables = variables[variables.size()-2];
-	BlockVariableMap &block_variables = variables.back();
-	for(BlockVariableMap::iterator i=block_variables.begin(); i!=block_variables.end(); ++i)
+	for(BlockVariableMap::const_iterator i=other_vars.begin(); i!=other_vars.end(); ++i)
 	{
-		if(i->second.local)
-		{
-			if(!i->second.referenced && !i->first.chain_len)
-				unused_nodes.insert(i->first.declaration);
-			/* Any unreferenced assignments when a variable runs out of scope
-			become unused. */
-			clear_assignments(i->second, true);
-			continue;
-		}
-
-		BlockVariableMap::iterator j = parent_variables.find(i->first);
-		if(j==parent_variables.end())
-			parent_variables.insert(*i);
-		else
+		BlockVariableMap::iterator j = variables.find(i->first);
+		if(j!=variables.end())
 		{
-			// Merge a non-local variable's state into the parent scope.
-			if(i->second.referenced || !i->second.conditionally_assigned)
-				clear_assignments(j->second, !i->second.referenced);
-			j->second.conditionally_assigned = i->second.conditionally_assigned;
+			/* The merged blocks started as copies of each other so any common
+			assignments must be in the beginning. */
+			unsigned k = 0;
+			for(; (k<i->second.assignments.size() && k<j->second.assignments.size()); ++k)
+				if(i->second.assignments[k]!=j->second.assignments[k])
+					break;
+
+			// Remaining assignments are unique to each block; merge them.
+			j->second.assignments.insert(j->second.assignments.end(), i->second.assignments.begin()+k, i->second.assignments.end());
 			j->second.referenced |= i->second.referenced;
-			j->second.assignments.insert(j->second.assignments.end(), i->second.assignments.begin(), i->second.assignments.end());
 		}
+		else
+			variables.insert(*i);
 	}
-	variables.pop_back();
 }
 
 void UnusedVariableRemover::visit(Conditional &cond)
 {
 	cond.condition->visit(*this);
-	variables.push_back(BlockVariableMap());
+	BlockVariableMap saved_vars = variables;
 	cond.body.visit(*this);
-
-	BlockVariableMap if_variables;
-	swap(variables.back(), if_variables);
+	swap(saved_vars, variables);
 	cond.else_body.visit(*this);
 
-	// Combine variables from both branches.
-	BlockVariableMap &else_variables = variables.back();
-	for(BlockVariableMap::iterator i=else_variables.begin(); i!=else_variables.end(); ++i)
-	{
-		BlockVariableMap::iterator j = if_variables.find(i->first);
-		if(j!=if_variables.end())
-		{
-			// The variable was found in both branches.
-			i->second.assignments.insert(i->second.assignments.end(), j->second.assignments.begin(), j->second.assignments.end());
-			i->second.conditionally_assigned |= j->second.conditionally_assigned;
-			if_variables.erase(j);
-		}
-		else
-			// Mark variables found in only one branch as conditionally assigned.
-			i->second.conditionally_assigned = true;
-	}
-
-	/* Move variables which were only used in the if block into the combined
-	block. */
-	for(BlockVariableMap::iterator i=if_variables.begin(); i!=if_variables.end(); ++i)
-	{
-		i->second.conditionally_assigned = true;
-		else_variables.insert(*i);
-	}
-
-	merge_down_variables();
+	/* Visible assignments after the conditional is the union of those visible
+	at the end of the if and else blocks.  If there was no else block, then it's
+	the union of the if block and the state before it. */
+	merge_variables(saved_vars);
 }
 
 void UnusedVariableRemover::visit(Iteration &iter)
 {
-	variables.push_back(BlockVariableMap());
+	BlockVariableMap saved_vars = variables;
 	TraversingVisitor::visit(iter);
-	merge_down_variables();
+
+	/* Merge assignments from the iteration, without clearing previous state.
+	Further analysis is needed to determine which parts of the iteration body
+	are always executed, if any. */
+	merge_variables(saved_vars);
 }
 
 
diff --git a/source/glsl/optimize.h b/source/glsl/optimize.h
index 26076452..5448ea7c 100644
--- a/source/glsl/optimize.h
+++ b/source/glsl/optimize.h
@@ -184,27 +184,36 @@ statements where the result is not used are also removed. */
 class UnusedVariableRemover: private TraversingVisitor
 {
 private:
+	struct AssignmentInfo
+	{
+		Node *node;
+		Assignment::Target target;
+		std::vector<Node *> used_by;
+
+		AssignmentInfo(): node(0) { }
+	};
+
 	struct VariableInfo
 	{
-		std::vector<Node *> assignments;
-		bool local;
+		InterfaceBlock *interface_block;
+		std::vector<AssignmentInfo *> assignments;
+		bool initialized;
 		bool output;
-		bool conditionally_assigned;
 		bool referenced;
-		InterfaceBlock *interface_block;
 
-		VariableInfo();
+		VariableInfo(): interface_block(0), initialized(false), output(false), referenced(false) { }
 	};
 
-	typedef std::map<Assignment::Target, VariableInfo> BlockVariableMap;
+	typedef std::map<Statement *, VariableInfo> BlockVariableMap;
 
 	Stage *stage;
-	std::set<Node *> unused_nodes;
-	std::vector<BlockVariableMap> variables;
+	BlockVariableMap variables;
+	std::list<AssignmentInfo> assignments;
 	InterfaceBlock *interface_block;
 	Assignment *r_assignment;
 	bool assignment_target;
 	bool r_side_effects;
+	std::set<Node *> unused_nodes;
 
 public:
 	UnusedVariableRemover();
@@ -212,14 +221,13 @@ public:
 	bool apply(Stage &);
 
 private:
-	void reference_used(Statement &);
+	void referenced(const Assignment::Target &, Node &);
 	virtual void visit(VariableReference &);
 	virtual void visit(InterfaceBlockReference &);
 	virtual void visit(UnaryExpression &);
 	virtual void visit(BinaryExpression &);
 	virtual void visit(Assignment &);
-	void record_assignment(const Assignment::Target &, Node &, bool);
-	void clear_assignments(VariableInfo &, bool);
+	void record_assignment(const Assignment::Target &, Node &);
 	virtual void visit(FunctionCall &);
 	virtual void visit(ExpressionStatement &);
 	// Ignore structs because their members can't be accessed directly.
@@ -227,7 +235,7 @@ private:
 	virtual void visit(VariableDeclaration &);
 	virtual void visit(InterfaceBlock &);
 	virtual void visit(FunctionDeclaration &);
-	void merge_down_variables();
+	void merge_variables(const BlockVariableMap &);
 	virtual void visit(Conditional &);
 	virtual void visit(Iteration &);
 };
diff --git a/tests/glsl/variable_assignment_subscript.glsl b/tests/glsl/variable_assignment_subscript.glsl
new file mode 100644
index 00000000..801b82b7
--- /dev/null
+++ b/tests/glsl/variable_assignment_subscript.glsl
@@ -0,0 +1,61 @@
+uniform mat4 mvp;
+uniform int mask_index;
+
+#pragma MSP stage(vertex)
+layout(location=0) in vec4 position;
+layout(location=1) in vec4 color;
+out VertexOut
+{
+	vec4 color;
+	vec4 mask_color;
+} vs_out;
+void main()
+{
+	int index = mask_index%4;
+	vs_out.color = color;
+	vs_out.color[index] = 0.0;
+	vs_out.mask_color = vec4(1.0);
+	vs_out.mask_color[index] = 1.0;
+	gl_Position = mvp*position;
+}
+
+#pragma MSP stage(fragment)
+layout(location=0) out vec4 frag_color;
+void main()
+{
+	frag_color = vs_out.color+vs_out.mask_color;
+}
+
+/* Expected output: vertex
+uniform mat4 mvp;
+uniform int mask_index;
+layout(location=0) in vec4 position;
+layout(location=1) in vec4 color;
+out VertexOut
+{
+	vec4 color;
+	vec4 mask_color;
+} vs_out;
+void main()
+{
+	int index = mask_index%4;
+	vs_out.color = color;
+	vs_out.color[index] = 0.0;
+	vs_out.mask_color = vec4(1.0);
+	vs_out.mask_color[index] = 1.0;
+	gl_Position = mvp*position;
+}
+/*
+
+/* Expected output: fragment
+layout(location=0) out vec4 frag_color;
+in VertexOut
+{
+	vec4 color;
+	vec4 mask_color;
+} vs_out;
+void main()
+{
+	frag_color = vs_out.color+vs_out.mask_color;
+}
+*/