Subzero: Provide a macro for iterating over instruction variables.

src/IceRegAlloc.cpp

Index: src/IceRegAlloc.cpp
diff --git a/src/IceRegAlloc.cpp b/src/IceRegAlloc.cpp
index 2c6d742c4dcd62c687892a06d438ce61279fc655..cad7fb582e8e329676d16e1916358e568047ed76 100644
--- a/src/IceRegAlloc.cpp
+++ b/src/IceRegAlloc.cpp
@@ -18,6 +18,7 @@
 #include "IceCfg.h"
 #include "IceCfgNode.h"
 #include "IceInst.h"
+#include "IceInstVarIter.h"
 #include "IceOperand.h"
 #include "IceTargetLowering.h"
 
@@ -180,16 +181,11 @@ void LinearScan::initForInfOnly() {
           }
         }
       }
-      for (SizeT I = 0; I < Inst.getSrcSize(); ++I) {
-        Operand *Src = Inst.getSrc(I);
-        SizeT NumVars = Src->getNumVars();
-        for (SizeT J = 0; J < NumVars; ++J) {
-          const Variable *Var = Src->getVar(J);
-          if (Var->getIgnoreLiveness())
-            continue;
-          if (Var->hasReg() || Var->mustHaveReg())
-            LREnd[Var->getIndex()] = Inst.getNumber();
-        }
+      FOREACH_VAR_IN_INST(Var, Inst) {

[ascull, 2015/08/31 19:03:17]

Looking at how this is used I was a bit confused as I expected Var to already
exist as a variable. The Linux kernel has some macro'd for loops
(https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each.html) and you
have to pass in the variable to use which solves the problem but leaves the
variable in the outer scope. But you still have the identifier name to used for
uniqueness.

I can't think how you can do it with:

  FOREACH_VAR_IN_INST(Variable *Var, Inst)

I get get stuck with '*Var' and can't work out how to strip the *.

[John, 2015/08/31 19:11:05]

The header file declaring the macros states that Var needs to be an undefined
id, but self documenting code would be better.
This looks redundant -- Var is always a Variable *.

Besides, how do you get rid of 'Variable *'?

[ascull, 2015/08/31 19:28:05]

It's not redundant as it shows the declaration of the variable instead of it
magically appearing. Being a C-style language comes with the expectation of
there being a clear declaration for variables.
 
As I've said both above and below, I'm not sure if this will work or how to do
it. I can get rid of 'Variable ' but not the *. I was just suggesting it as it
feels more similar to the native for each and you might have known some
preprocessor trick to get it to work.

[John, 2015/08/31 22:07:03]

It is redundant -- Types in variable declarations are always redundant when you
initialize said variable -- the compiler knows what type the variable is so
re-stating it is redundant. I did not say "wrong", "ugly", or "poor style." :)
Oh, mistyped. I wanted to know how you could get rid of the 'Variable ' (not the
*)

[ascull, 2015/08/31 22:31:56]

#define remove_variable(decl) abc##decl
#define abc_Variable /* Note: this is empty */

`remove_variable(Variable *Var)` -> `abc_Variable *Var` -> `*Var`

The limitation is that it requires you to know all the possibilities ahead of
time.

+        if (Var->getIgnoreLiveness())
+          continue;
+        if (Var->hasReg() || Var->mustHaveReg())
+          LREnd[Var->getIndex()] = Inst.getNumber();
       }
     }
   }
@@ -298,14 +294,9 @@ void LinearScan::addSpillFill(IterationState &Iter) {
       // Remove from RegMask any physical registers referenced during Cur's live
       // range.  Start looking after SpillPoint gets set, i.e. once Cur's live
       // range begins.
-      for (SizeT i = 0; i < I->getSrcSize(); ++i) {
-        Operand *Src = I->getSrc(i);
-        SizeT NumVars = Src->getNumVars();
-        for (SizeT j = 0; j < NumVars; ++j) {
-          const Variable *Var = Src->getVar(j);
-          if (Var->hasRegTmp())
-            Iter.RegMask[Var->getRegNumTmp()] = false;
-        }
+      FOREACH_VAR_IN_INST(Var, *I) {
+        if (Var->hasRegTmp())
+          Iter.RegMask[Var->getRegNumTmp()] = false;
       }
     }
   }