Subzero: Control whether deleted instructions are retained.

src/IceCfgNode.cpp

 //===- subzero/src/IceCfgNode.cpp - Basic block (node) implementation -----===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 34 matching lines @@
     if (!Insts.empty()) {
       Func->setError("Phi instruction added to the middle of a block");
       return;
     }
     Phis.push_back(Phi);
   } else {
     Insts.push_back(Instr);
   }
 }
 
-// Renumbers the non-deleted instructions in the node. This needs to be done in
-// preparation for live range analysis. The instruction numbers in a block must
-// be monotonically increasing. The range of instruction numbers in a block,
-// from lowest to highest, must not overlap with the range of any other block.
+namespace {
+template <typename List> void removeDeletedAndRenumber(List *L, Cfg *Func) {

[John, 2016/03/14 13:27:15]

You don't need a template here -- L is always an InstList.

[Jim Stichnoth, 2016/03/14 13:51:15]

In one case it's InstList, in another case it's PhiList.

Now I realize that InstList and PhiList happen to be the same.  I actually
wanted PhiList to be ilist<InstPhi>, but SFINAE issues.  (This is documented in
IceDefs.h.)  I'd prefer to keep this templated for if/when PhiList diverges.

[John, 2016/03/14 14:55:50]

Well, not a big deal --hence the previous LGTM, but this is yet another case of
convoluted code that does not need to be so...

The problem is not SFINAE: every Ice::Inst has ilist methods in it -- hooray for
intrusive lists! --, and they all return Inst * (because the Ice::Inst inherits
from llvm::ilist<Inst>)

if you

using PhiList = llvm::ilist<InstPhi>

the for loops become:

using llvm::ilist;
for (ilist<PhiInst>::iterator it = Phis.begin(); it != Phis.end(); ++it) {
}

now, because

class Inst : public ilis<Inst>;
class InstHighLevel : public Inst;
class InstPhi : public InstHistLevel;

typedecl(Phis.begin()) == ilist<Inst>::iterator,

and hence all hell break loose.

If phi lists are so special, maybe they should not inherit from Inst.

As I said, I **personally** would not create the template here -- I would prefer
to fix this if the two ever diverge -- but really, this is a matter of taste,
and the current solution has no drawbacks.

+  const bool DoDelete =
+      BuildDefs::minimal() || !GlobalContext::getFlags().getKeepDeletedInsts();
+  auto I = L->begin(), E = L->end(), Next = I;
+  for (++Next; I != E; I = Next++) {

[Eric Holk, 2016/03/14 13:20:14]

Does a ranged for loop work here?

[Jim Stichnoth, 2016/03/14 13:51:16]

I don't think so, because erase(I) invalidates iterator I, so the implicit ++I
at the end of the loop body is invalid.

One thought is something like this:

  for (auto &I : *L) {
    if (...) {
      auto Prev = I;
      --Prev
      L->erase(I);
      I = Prev;
    } else {
      I->renumber(Func);
    }
  }

except this breaks when the very first instruction is to be deleted because you
can't back up past the start of the list.  So you could start it off with a
separate loop that erases the initial deleted items.

I think the original implementation is cleaner. :)

[Eric Holk, 2016/03/14 14:01:03]

Ah, I see. I figured you had a good reason :)

+    if (DoDelete && I->isDeleted()) {
+      L->erase(I);
+    } else {
+      I->renumber(Func);
+    }
+  }
+}
+} // end of anonymous namespace
+
 void CfgNode::renumberInstructions() {
   InstNumberT FirstNumber = Func->getNextInstNumber();
-  for (Inst &I : Phis)
-    I.renumber(Func);
-  for (Inst &I : Insts)
-    I.renumber(Func);
+  removeDeletedAndRenumber(&Phis, Func);
+  removeDeletedAndRenumber(&Insts, Func);
   InstCountEstimate = Func->getNextInstNumber() - FirstNumber;
 }
 
 // When a node is created, the OutEdges are immediately known, but the InEdges
 // have to be built up incrementally. After the CFG has been constructed, the
 // computePredecessors() pass finalizes it by creating the InEdges list.
 void CfgNode::computePredecessors() {
   for (CfgNode *Succ : OutEdges)
     Succ->InEdges.push_back(this);
 }
@@ skipping 1365 matching lines @@
   auto *Instr = InstIntrinsicCall::create(
       Func, 5, Func->makeVariable(IceType_i64), RMWI64Name, Info->Info);
   Instr->addArg(AtomicRMWOp);
   Instr->addArg(Counter);
   Instr->addArg(One);
   Instr->addArg(OrderAcquireRelease);
   Insts.push_front(Instr);
 }
 
 } // end of namespace Ice