[MLton-commit] r7503

[Matthew Fluet]

Fixed bug with treatment of nan in common subexpression elimination SSA optimization.

Common subexpression elimination is implemented with a pre-order
traversal of the dominator tree and a (mutable) hashtable of
canonicalized expressions.  Since the hashtable is mutable, after
visiting all of the children of a block and before returning from the
block, all entries added to the hashtable by this block must be
removed.  Previously, the removal was performed by:

                  List.foreach
                  (!removes, fn {var, exp, hash} =>
                   HashSet.remove
                   (table, hash, fn {var = var', exp = exp', ...} =>
                    Var.equals (var, var') andalso
                    Exp.equals (exp, exp')));

However, if one of the entries to be removed is a RealX constant
denoting nan, then Exp.equals returns false (because the underlying
RealX.equals uses Real{32,64}.== and any comparison with nan returns
false).  HashSet.remove fails (with List.removeFirst) when the entry
to be removed is not found.

Since a variable can be associated with exactly one canonicalized
expression, the "Var.equals (var, var')" implies the equality (and
identity) of the exp and exp'.  Hence, replace the above with:

                  List.foreach
                  (!removes, fn {var, exp, hash} =>
                   HashSet.remove
                   (table, hash, fn {var = var', ...} =>
                    Var.equals (var, var')));

Note, however, that this highlights a deficiency of common
subexpression elimination --- it will never eliminate a nan value,
since it will never identify two nan expressions as equal.

The reason that this bug hadn't been identified before is that it is
only triggered by a nan value appearing as a non-global statement in a
program.  For example, the Basis Library has an explicit calculation
of nan:
      val nan = posInf + negInf
This is constant folded early in the compilation and then constant
propagated into a global.  (The deficiency noted above also applies to
constant propagation, which could result in multiple nan values being
propagated into globals.)  Since globals never go out of scope, there
is no removal of the hash table entries created by the globals.
However, code like:
      val r = !(ref 0.0) / !(ref 0.0)
is not constant-folded until after the local ref optimization, which
runs after constant propagation.  Therefore, the resulting nan value
remains a non-global statement, and triggers the bug.

Interestingly, the original CSE pass did not use the
"Exp.equals (exp, exp')" condition for the hash table removal.  It was
added by r1586, without a clear justification.

Thanks to Alexandre Hamez for the bug report.
----------------------------------------------------------------------

U   mlton/trunk/doc/changelog
U   mlton/trunk/mlton/ssa/common-subexp.fun
A   mlton/trunk/regression/common-subexp0.ok
A   mlton/trunk/regression/common-subexp0.sml

----------------------------------------------------------------------

Modified: mlton/trunk/doc/changelog
===================================================================
--- mlton/trunk/doc/changelog	2011-02-18 21:18:47 UTC (rev 7502)
+++ mlton/trunk/doc/changelog	2011-02-18 21:18:51 UTC (rev 7503)
@@ -1,5 +1,9 @@
 Here are the changes from version 2010608 to version YYYYMMDD.
 
+* 2011-02-18
+   - Fixed bug with treatment of nan in common subexpression
+     elimination SSA optimization.
+
 * 2011-02-17
    - Fixed bug in translation from SSA2 to RSSA with weak pointers.
 

Modified: mlton/trunk/mlton/ssa/common-subexp.fun
===================================================================
--- mlton/trunk/mlton/ssa/common-subexp.fun	2011-02-18 21:18:47 UTC (rev 7502)
+++ mlton/trunk/mlton/ssa/common-subexp.fun	2011-02-18 21:18:51 UTC (rev 7503)
@@ -1,4 +1,4 @@
-(* Copyright (C) 2009 Matthew Fluet.
+(* Copyright (C) 2009,2011 Matthew Fluet.
  * Copyright (C) 1999-2006 Henry Cejtin, Matthew Fluet, Suresh
  *    Jagannathan, and Stephen Weeks.
  * Copyright (C) 1997-2000 NEC Research Institute.
@@ -299,11 +299,10 @@
                                                      Exp.layout exp]) (!removes)])
                    end);
                   List.foreach 
-                  (!removes, fn {var, exp, hash} =>
+                  (!removes, fn {var, hash, ...} =>
                    HashSet.remove
-                   (table, hash, fn {var = var', exp = exp', ...} =>
-                    Var.equals (var, var') andalso 
-                    Exp.equals (exp, exp')));
+                   (table, hash, fn {var = var', ...} =>
+                    Var.equals (var, var')));
                   diag "removed"
                end
             val _ =

Added: mlton/trunk/regression/common-subexp0.ok
===================================================================
--- mlton/trunk/regression/common-subexp0.ok	2011-02-18 21:18:47 UTC (rev 7502)
+++ mlton/trunk/regression/common-subexp0.ok	2011-02-18 21:18:51 UTC (rev 7503)
@@ -0,0 +1 @@
+nan

Added: mlton/trunk/regression/common-subexp0.sml
===================================================================
--- mlton/trunk/regression/common-subexp0.sml	2011-02-18 21:18:47 UTC (rev 7502)
+++ mlton/trunk/regression/common-subexp0.sml	2011-02-18 21:18:51 UTC (rev 7503)
@@ -0,0 +1,2 @@
+val x = !(ref 0.0) / !(ref 0.0)
+val _ = print (concat [Real.toString x, "\n"])