Enable Local CSE by default

src/IceCfg.cpp

Index: src/IceCfg.cpp
diff --git a/src/IceCfg.cpp b/src/IceCfg.cpp
index 23c363fa7791938a5a05a0edfc6adaf72167f48b..e14a7905321f778c597b8f40e349226fa42283e9 100644
--- a/src/IceCfg.cpp
+++ b/src/IceCfg.cpp
@@ -514,19 +514,23 @@ void Cfg::shuffleNodes() {
   dump("After basic block shuffling");
 }
 
-void Cfg::localCSE() {
+void Cfg::localCSE(const bool NoSSA) {
   // Performs basic-block local common-subexpression elimination
   // If we have
   // t1 = op b c
   // t2 = op b c
   // This pass will replace future references to t2 in a basic block by t1
   // Points to note:
-  // 1. Does not assume SSA, but not tested on non-SSA input yet as it is run
-  //    at the beginning.
+  // 1. Assumes SSA by default. To change this, use -lcse=no-ssa
+  //      This is needed if this pass is moved to a point later in the pipeline.
+  //      If variables have a single definition (in the node), CSE can work just
+  //      on the basis of an equality compare on instructions (sans Dest). When
+  //      variables can be updated (hence, non-SSA) the result of a previous
+  //      instruction which used that variable as an operand can not be reused.
   // 2. Leaves removal of instructions to DCE.
   // 3. Only enabled on arithmetic instructions. pnacl-clang (-O2) is expected
   //    to take care of cases not arising from GEP simplification.
-  // 4. By default, two passes are made over each basic block. Control this
+  // 4. By default, a single pass is made over each basic block. Control this
   //    with -lcse-max-iters=N
 
   TimerMarker T(TimerStack::TT_localCse, this);
@@ -581,35 +585,33 @@ void Cfg::localCSE() {
     // memory but will be slower i.e map of Instruction -> Set of Variables
 
     CfgUnorderedMap<Variable *, std::vector<Inst *>, VariableHash> Dependency;
-    // Not necessary for SSA, still keeping it in case this pass is not run at
-    // the beginning. Remove to improve performace.
+    // Not necessary for SSA

[Jim Stichnoth, 2016/07/29 14:49:02]

Can you describe this with respect to the arg name/condition?  Also, might note
that it should consume minimal memory when unused.

[manasijm, 2016/08/01 17:39:23]

Done.

 
     int IterCount = getFlags().getLocalCseMaxIterations();
 
     while (IterCount--) {

[Jim Stichnoth, 2016/07/29 14:49:02]

When I look at this loop, I get nervous about off-by-one errors, pre-increment
versus post-increment, signed versus unsigned, etc.  How about a more standard
looping structure?

const uint32_t IterCount = ...;
for (uint32_t i = 0; i < IterCount; ++i) { ... }

This also suggests changing the flag in IceClFlags.def from int to uint32_t.

[manasijm, 2016/08/01 17:39:23]

Done.

-      // TODO : Stats on IterCount -> performance
+      // TODO(manasijm): Stats on IterCount -> performance
       for (Inst &Instr : Node->getInsts()) {
         if (Instr.isDeleted() || !llvm::isa<InstArithmetic>(&Instr))
           continue;
+        if (NoSSA) {
+          // Invalidate replacements
+          auto Iter = Replacements.find(Instr.getDest());
+          if (Iter != Replacements.end()) {
+            Replacements.erase(Iter);
+          }
 
-        // Invalidate replacements
-        auto Iter = Replacements.find(Instr.getDest());
-        if (Iter != Replacements.end()) {
-          Replacements.erase(Iter);
-        }
-
-        // Invalidate 'seen' instructions whose operands were just updated.
-        auto DepIter = Dependency.find(Instr.getDest());
-        if (DepIter != Dependency.end()) {
-          for (auto DepInst : DepIter->second) {
-            Seen.erase(DepInst);
+          // Invalidate 'seen' instructions whose operands were just updated.
+          auto DepIter = Dependency.find(Instr.getDest());
+          if (DepIter != Dependency.end()) {
+            for (auto *DepInst : DepIter->second) {
+              Seen.erase(DepInst);
+            }
           }
         }
-        // The above two can be removed if SSA is assumed.
 
         // Replace - doing this before checking for repetitions might enable
-        // more
-        // optimizations
+        // more optimizations
         for (SizeT i = 0; i < Instr.getSrcSize(); ++i) {
           auto *Opnd = Instr.getSrc(i);
           if (auto *Var = llvm::dyn_cast<Variable>(Opnd)) {
@@ -627,11 +629,13 @@ void Cfg::localCSE() {
         } else { // new
           Seen.insert(&Instr);
 
-          // Update dependencies
-          for (SizeT i = 0; i < Instr.getSrcSize(); ++i) {
-            auto *Opnd = Instr.getSrc(i);
-            if (auto *Var = llvm::dyn_cast<Variable>(Opnd)) {
-              Dependency[Var].push_back(&Instr);
+          if (NoSSA) {
+            // Update dependencies
+            for (SizeT i = 0; i < Instr.getSrcSize(); ++i) {
+              auto *Opnd = Instr.getSrc(i);
+              if (auto *Var = llvm::dyn_cast<Variable>(Opnd)) {
+                Dependency[Var].push_back(&Instr);
+              }
             }
           }
         }