Subzero. ARM32. Vector lowering. Subtract.

src/IceInst.h

 //===- subzero/src/IceInst.h - High-level instructions ----------*- C++ -*-===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 58 matching lines @@
     JumpTable,    // not part of LLVM/PNaCl bitcode
     // Anything >= Target is an InstTarget subclass. Note that the value-spaces
     // are shared across targets. To avoid confusion over the definition of
     // shared values, an object specific to one target should never be passed
     // to a different target.
     Target,
     Target_Max = std::numeric_limits<uint8_t>::max(),
   };
   static_assert(Target <= Target_Max, "Must not be above max.");
   InstKind getKind() const { return Kind; }
+  IceString getInstName() const {

[John, 2016/01/26 19:07:17]

Can you move the method to IceInst.cpp?

[John, 2016/01/26 19:07:17]

this is really useful, but does not belong in this patch. can you move it to its
own cl?

This is also going to increase what's called minimal mode (make MINIMAL=1),
which builds the binary shipped to clients. I would suggest you to add a guard

  if (!BuildDefs::dump())
    return "";

I am not sure getInstName() conveys the purpose of this method. It returns the
instruction **type**, but its name, converted to a string, but I can't think of
anything better.

[Jim Stichnoth, 2016/01/26 19:13:10]

Could you also change the dump() methods in IceInst.cpp to use this method
instead of hard-coded strings?  This would also mean lower-casing the returned
strings in this method.

[Jim Stichnoth, 2016/01/26 19:13:10]

Add the following to the beginning of this method:

if (!BuildDefs::dump())
  return "???";

or something to that effect.  This reduces the translator size in a MINIMAL
build.

[Eric Holk, 2016/01/26 22:49:24]

Sure.

[Eric Holk, 2016/01/26 22:49:24]

Yup, I'm working on that now.
Sure.
Yeah, I don't really like the name either. What about getKindAsString()? Or
maybe even just toString()?

[Eric Holk, 2016/01/26 22:49:24]

I made some of the obvious changes, but it looks like more generally we might
just be able to get rid of most of the special cases of dump.

+    switch (Kind) {
+    case Unreachable:

[John, 2016/01/26 19:07:17]

I would suggest the following:

switch (Kind) {
#define CASE(InstKind) case InstKind: return #InstKind;
  CASE(Unreachable)
  CASE(Alloca)
  CASE(Arithmetic)
  ...
  CASE(JumpTable)
  default:
  CASE(Target)
#undef X
}

the default is very important

+      return "Unreachable";
+    case Alloca:
+      return "Alloca";
+    case Arithmetic:
+      return "Arithmetic";
+    case Br:
+      return "Br";
+    case Call:
+      return "Call";
+    case Cast:
+      return "Cast";
+    case ExtractElement:
+      return "ExtractElement";
+    case Fcmp:
+      return "Fcmp";
+    case Icmp:
+      return "Icmp";
+    case IntrinsicCall:
+      return "IntrinsicCall";
+    case InsertElement:
+      return "InsertElement";
+    case Load:
+      return "Load";
+    case Phi:
+      return "Phi";
+    case Ret:
+      return "Ret";
+    case Select:
+      return "Select";
+    case Store:
+      return "Store";
+    case Switch:
+      return "Switch";
+    case Assign:
+      return "Assign";
+    case BundleLock:
+      return "BundleLock";
+    case BundleUnlock:
+      return "BundleUnlock";
+    case FakeDef:
+      return "FakeDef";
+    case FakeUse:
+      return "FakeUse";
+    case FakeKill:
+      return "FakeKill";
+    case JumpTable:
+      return "JumpTable";
+    default:
+      return "Target";

[Jim Stichnoth, 2016/01/26 19:13:10]

Add an assert:
  assert(Kind >= Target);
just to make sure that newly added high-level instruction kinds don't forget to
add their string here.

[Eric Holk, 2016/01/26 22:49:24]

Okay.

+    }
+  }
 
   InstNumberT getNumber() const { return Number; }
   void renumber(Cfg *Func);
   enum {
     NumberDeleted = -1,
     NumberSentinel = 0,
     NumberInitial = 2,
     NumberExtended = NumberInitial - 1
   };
 
@@ skipping 192 matching lines @@
 #undef X
         _num
   };
 
   static InstArithmetic *create(Cfg *Func, OpKind Op, Variable *Dest,
                                 Operand *Source1, Operand *Source2) {
     return new (Func->allocate<InstArithmetic>())
         InstArithmetic(Func, Op, Dest, Source1, Source2);
   }
   OpKind getOp() const { return Op; }
+
+  IceString getInstName() const {

[John, 2016/01/26 19:07:17]

add the

if (!BuildDefs::dump()) return "";

and move this to the .cpp file.

[Jim Stichnoth, 2016/01/26 19:13:10]

Are you shadowing a nonvirtual method name in a base class?  That seems a little
dangerous or error-prone.  Should this be virtual, with override or final here?

[Eric Holk, 2016/01/26 22:49:24]

Okay.

[Eric Holk, 2016/01/26 22:49:24]

I was thinking the same thing. I made it virtual.

+    switch (Op) {
+#define X(tag, str, commutative)                                               \
+  case tag:                                                                    \
+    return str;
+      ICEINSTARITHMETIC_TABLE
+#undef X
+    default:
+      return "Unknown";
+    }
+  }
+
   static const char *getOpName(OpKind Op);
   bool isCommutative() const;
   void dump(const Cfg *Func) const override;
   static bool classof(const Inst *Inst) {
     return Inst->getKind() == Arithmetic;
   }
 
 private:
   InstArithmetic(Cfg *Func, OpKind Op, Variable *Dest, Operand *Source1,
                  Operand *Source2);
@@ skipping 688 matching lines @@
   static void noteHead(Ice::Inst *, Ice::Inst *) {}
   void deleteNode(Ice::Inst *) {}
 
 private:
   mutable ilist_half_node<Ice::Inst> Sentinel;
 };
 
 } // end of namespace llvm
 
 #endif // SUBZERO_SRC_ICEINST_H