From aa9873652630db493b5bd9faee4117e9c30ef226 Mon Sep 17 00:00:00 2001
From: Mikko Rasa <tdb@tdb.fi>
Date: Sun, 26 May 2019 00:00:54 +0300
Subject: [PATCH] Fix various issues with constant condition elimination

It used to optimize based on the initial values of variables, ignoring
the fact that they may change between loop iterations.  A stop-gap fix
was implemented in 8e14c29, but it prevented optimizations of some
conditionals that could safely have been removed.  It also caused the
optimizer to ignore any assignments made to any variables after
declaration when evaluating the loop condition, potentially causing
entire loops to be erroneously removed.  Finally, unary expressions were
not being handled so ++ and -- operators did not mark the variable as
modified.
---
 source/programcompiler.cpp | 64 +++++++++++++++++++++++++-------------
 source/programcompiler.h   |  6 ++--
 2 files changed, 46 insertions(+), 24 deletions(-)

diff --git a/source/programcompiler.cpp b/source/programcompiler.cpp
index a1be91ca..1f014f72 100644
--- a/source/programcompiler.cpp
+++ b/source/programcompiler.cpp
@@ -1210,14 +1210,12 @@ void ProgramCompiler::FunctionInliner::visit(Return &ret)
 
 ProgramCompiler::ExpressionEvaluator::ExpressionEvaluator():
 	variable_values(0),
-	const_only(false),
 	result(0.0f),
 	result_valid(false)
 { }
 
-ProgramCompiler::ExpressionEvaluator::ExpressionEvaluator(const ValueMap *v, bool c):
-	variable_values(v),
-	const_only(c),
+ProgramCompiler::ExpressionEvaluator::ExpressionEvaluator(const ValueMap &v):
+	variable_values(&v),
 	result(0.0f),
 	result_valid(false)
 { }
@@ -1242,8 +1240,6 @@ void ProgramCompiler::ExpressionEvaluator::visit(VariableReference &var)
 {
 	if(!var.declaration)
 		return;
-	if(const_only && !var.declaration->constant)
-		return;
 
 	if(variable_values)
 	{
@@ -1304,7 +1300,7 @@ void ProgramCompiler::ExpressionEvaluator::visit(BinaryExpression &binary)
 
 ProgramCompiler::ConstantConditionEliminator::ConstantConditionEliminator():
 	scope_level(0),
-	in_loop(false)
+	record_only(false)
 { }
 
 void ProgramCompiler::ConstantConditionEliminator::visit(Block &block)
@@ -1316,6 +1312,13 @@ void ProgramCompiler::ConstantConditionEliminator::visit(Block &block)
 		variable_values.erase(i->second);
 }
 
+void ProgramCompiler::ConstantConditionEliminator::visit(UnaryExpression &unary)
+{
+	if(VariableReference *var = dynamic_cast<VariableReference *>(unary.expression.get()))
+		if(unary.oper=="++" || unary.oper=="--")
+			variable_values.erase(var->declaration);
+}
+
 void ProgramCompiler::ConstantConditionEliminator::visit(Assignment &assign)
 {
 	variable_values.erase(assign.target_declaration);
@@ -1329,30 +1332,49 @@ void ProgramCompiler::ConstantConditionEliminator::visit(VariableDeclaration &va
 
 void ProgramCompiler::ConstantConditionEliminator::visit(Conditional &cond)
 {
-	ExpressionEvaluator eval(&variable_values, in_loop);
-	cond.condition->visit(eval);
-	if(eval.result_valid)
-		flatten_block(eval.result ? cond.body : cond.else_body);
-	else
-		TraversingVisitor::visit(cond);
+	if(!record_only)
+	{
+		ExpressionEvaluator eval(variable_values);
+		cond.condition->visit(eval);
+		if(eval.result_valid)
+		{
+			flatten_block(eval.result ? cond.body : cond.else_body);
+			return;
+		}
+	}
+
+	TraversingVisitor::visit(cond);
 }
 
 void ProgramCompiler::ConstantConditionEliminator::visit(Iteration &iter)
 {
-	// XXX Should this not visit init_statement first?
-	if(iter.condition)
+	if(!record_only)
 	{
-		ExpressionEvaluator eval(0, in_loop);
-		iter.condition->visit(eval);
-		if(eval.result_valid && !eval.result)
+		if(iter.condition)
 		{
-			remove_node = true;
-			return;
+			/* If the loop condition is always false on the first iteration, the
+			entire loop can be removed */
+			if(iter.init_statement)
+				iter.init_statement->visit(*this);
+			ExpressionEvaluator eval(variable_values);
+			iter.condition->visit(eval);
+			if(eval.result_valid && !eval.result)
+			{
+				remove_node = true;
+				return;
+			}
 		}
+
+		/* Record all assignments that occur inside the loop body so those
+		variables won't be considered as constant */
+		SetFlag set_record(record_only);
+		TraversingVisitor::visit(iter);
 	}
 
-	SetFlag set_loop(in_loop);
 	TraversingVisitor::visit(iter);
+
+	if(VariableDeclaration *init_decl = dynamic_cast<VariableDeclaration *>(iter.init_statement.get()))
+		variable_values.erase(init_decl);
 }
 
 
diff --git a/source/programcompiler.h b/source/programcompiler.h
index 2c8b33b6..461fac9a 100644
--- a/source/programcompiler.h
+++ b/source/programcompiler.h
@@ -226,12 +226,11 @@ private:
 		typedef std::map<ProgramSyntax::VariableDeclaration *, ProgramSyntax::Expression *> ValueMap;
 
 		const ValueMap *variable_values;
-		bool const_only;
 		float result;
 		bool result_valid;
 
 		ExpressionEvaluator();
-		ExpressionEvaluator(const ValueMap *, bool);
+		ExpressionEvaluator(const ValueMap &);
 
 		using ProgramSyntax::NodeVisitor::visit;
 		virtual void visit(ProgramSyntax::Literal &);
@@ -244,13 +243,14 @@ private:
 	struct ConstantConditionEliminator: BlockModifier
 	{
 		unsigned scope_level;
-		bool in_loop;
+		bool record_only;
 		ExpressionEvaluator::ValueMap variable_values;
 
 		ConstantConditionEliminator();
 
 		using Visitor::visit;
 		virtual void visit(ProgramSyntax::Block &);
+		virtual void visit(ProgramSyntax::UnaryExpression &);
 		virtual void visit(ProgramSyntax::Assignment &);
 		virtual void visit(ProgramSyntax::VariableDeclaration &);
 		virtual void visit(ProgramSyntax::Conditional &);
-- 
2.45.2