Subzero: Control whether deleted instructions are retained.

src/IceCfgNode.cpp

Index: src/IceCfgNode.cpp
diff --git a/src/IceCfgNode.cpp b/src/IceCfgNode.cpp
index 0729585bf14f8bcad1fe5b84c8ac04f138f6c700..212f742c53959789acaf5a036fd548303ea96eff 100644
--- a/src/IceCfgNode.cpp
+++ b/src/IceCfgNode.cpp
@@ -52,16 +52,25 @@ void CfgNode::appendInst(Inst *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;
 }