From 8e34cbdf15b1ac1469b8716cbc3c2c6e48e62bc1 Mon Sep 17 00:00:00 2001
From: Bert Maher <bertrand@fb.com>
Date: Fri, 31 May 2013 09:01:57 -0700
Subject: [PATCH] Allow CSE of ArrayGet by converting to IncRef

ArrayGet has fairly minimal side-effects: IncRef of its
result, and raising a warning if the element is undefined.  To CSE
ArrayGet, replace it with an IncRef of its destination.  Raising
multiple warnings for repeated accesses doesn't seem to have much
value, so just ignore them.
---
 hphp/runtime/vm/translator/hopt/ir.cpp           |  1 -
 hphp/runtime/vm/translator/hopt/ir.h             |  2 +-
 hphp/runtime/vm/translator/hopt/tracebuilder.cpp |  8 +++++++-
 hphp/test/quick/array_cse.php                    | 10 ++++++++++
 hphp/test/quick/array_cse.php.expect             |  1 +
 5 files changed, 19 insertions(+), 3 deletions(-)
 create mode 100644 hphp/test/quick/array_cse.php
 create mode 100644 hphp/test/quick/array_cse.php.expect

diff --git a/hphp/runtime/vm/translator/hopt/ir.cpp b/hphp/runtime/vm/translator/hopt/ir.cpp
index 9e1fc4387..79fa020b8 100644
--- a/hphp/runtime/vm/translator/hopt/ir.cpp
+++ b/hphp/runtime/vm/translator/hopt/ir.cpp
@@ -363,7 +363,6 @@ bool IRInstruction::canCSE() const {
   // count or consume reference counts. CheckType is special because it can
   // refine a maybeCounted type to a notCounted type, so it logically consumes
   // and produces a reference without doing any work.
-  assert(!canCSE || !producesReference() || m_op == CheckType);
   assert(!canCSE || !consumesReferences() || m_op == CheckType);
   return canCSE && !mayReenterHelper();
 }
diff --git a/hphp/runtime/vm/translator/hopt/ir.h b/hphp/runtime/vm/translator/hopt/ir.h
index e465d1249..5e06f6cff 100644
--- a/hphp/runtime/vm/translator/hopt/ir.h
+++ b/hphp/runtime/vm/translator/hopt/ir.h
@@ -554,7 +554,7 @@ O_STK(ElemUX,              D(PtrToGen), C(TCA)                                \
                                           S(PtrToCell),VElem|E|N|Mem|Refs|Er) \
 O(ArrayGet,                    D(Cell), C(TCA)                                \
                                           S(Arr)                              \
-                                          S(Int,Str),    E|N|PRc|Refs|Mem|Er) \
+                                          S(Int,Str),     C|N|PRc|Refs|Mem|Er)\
 O(CGetElem,                    D(Cell), C(TCA)                                \
                                           S(PtrToGen)                         \
                                           S(Cell)                             \
diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp
index 73d7ed9ac..48bc66f5a 100644
--- a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp
+++ b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp
@@ -768,7 +768,13 @@ SSATmp* TraceBuilder::optimizeWork(IRInstruction* inst) {
       // Found a dominating instruction that can be used instead of inst
       FTRACE(1, "  {}cse found: {}\n",
              indent(), result->inst()->toString());
-      return result;
+      if (inst->producesReference()) {
+        // Replace with an IncRef
+        FTRACE(1, "  {}cse of refcount-producing instruction\n", indent());
+        return gen(IncRef, result);
+      } else {
+        return result;
+      }
     }
   }
 
diff --git a/hphp/test/quick/array_cse.php b/hphp/test/quick/array_cse.php
new file mode 100644
index 000000000..a4334e781
--- /dev/null
+++ b/hphp/test/quick/array_cse.php
@@ -0,0 +1,10 @@
+<?php
+// Copyright 2004-present Facebook. All Rights Reserved.
+
+function array_cse() {
+  $a = array(0,1,2,3,4);
+  $x = $a[2] + $a[2];
+  return $x;
+}
+
+var_dump(array_cse());
diff --git a/hphp/test/quick/array_cse.php.expect b/hphp/test/quick/array_cse.php.expect
new file mode 100644
index 000000000..d1ec06fdd
--- /dev/null
+++ b/hphp/test/quick/array_cse.php.expect
@@ -0,0 +1 @@
+int(4)