[PATCH] [opt] Devirtualize the SymbolBody type hierarchy and start compacting its members into the base class.

[Chandler Carruth]

I think I've addressed everything. PTAL.


================
Comment at: COFF/Symbols.cpp:46-48
@@ +45,5 @@
+  if (LK != RK) {
+    // Normalize so that the smaller kind is on the left.
+    if (LK > RK)
+      return -Other->compare(this);
+
----------------
ruiu wrote:
> Move this before "if (LK != RK)" to reduce nesting level a bit.
Done.

================
Comment at: COFF/Symbols.cpp:145-146
@@ +144,4 @@
+  switch (kind()) {
+  case DefinedBitcodeKind:
+    return cast<DefinedBitcode>(this)->getRVA();
+  case DefinedAbsoluteKind:
----------------
ruiu wrote:
> getRVA should never be called for bitcode symbols. You can remove this "case" and DefinedBitcode::getRVA.
Sure, I was letting the inliner do it, I'll do it directly and give a better message.

================
Comment at: COFF/Symbols.cpp:168-169
@@ +167,4 @@
+  switch (kind()) {
+  case DefinedBitcodeKind:
+    return cast<DefinedBitcode>(this)->getFileOff();
+  case DefinedAbsoluteKind:
----------------
ruiu wrote:
> Ditto
Done.

================
Comment at: COFF/Symbols.cpp:170-171
@@ +169,4 @@
+    return cast<DefinedBitcode>(this)->getFileOff();
+  case DefinedAbsoluteKind:
+    return cast<DefinedAbsolute>(this)->getFileOff();
+  case DefinedImportDataKind:
----------------
ruiu wrote:
> This can also be removed.
Similar to the above, done.

================
Comment at: COFF/Symbols.h:97-104
@@ -88,3 +96,10 @@
 
-private:
-  const Kind SymbolKind;
+  const unsigned SymbolKind : 8;
+  unsigned IsExternal : 1;
+
+  // This bit is used by the \c DefinedRegular subclass.
+  unsigned IsCOMDAT : 1;
+
+  // This bit is used by the \c DefinedBitcode subclass.
+  unsigned IsReplaceable : 1;
+
----------------
ruiu wrote:
> Does the use of bitfield here save space? I guess StringRef is 8 byte aligned on 64 bit platform, so we have 8 bytes for attributes. We have four fields, so we can use 1 byte for each.
The very next flag added would push us over 4 bytes though, and that would waste space on a 32-bit system. I'd like to pick a design that makes it easy to extend without having dramatic changes in efficiency.

================
Comment at: COFF/Symbols.h:98
@@ +97,3 @@
+  const unsigned SymbolKind : 8;
+  unsigned IsExternal : 1;
+
----------------
ruiu wrote:
> Can you use C++11 initializer?
> 
>   unsigned IsExternal : 1 = true;
I tried and it didn't compile. =/

================
Comment at: COFF/Symbols.h:114
@@ -97,3 +113,3 @@
 public:
-  Defined(Kind K) : SymbolBody(K) {}
+  Defined(Kind K, StringRef N = "") : SymbolBody(K, N) {}
 
----------------
ruiu wrote:
> Add explicit.
Done.

================
Comment at: COFF/Symbols.h:130
@@ +129,3 @@
+// Symbols defined via a COFF object file.
+class ObjectFileDefined : public Defined {
+  friend SymbolBody;
----------------
ruiu wrote:
> All other classes starts with Defined, so name this DefinedObject, DefinedFile or DefinedCOFF.
Done.

================
Comment at: COFF/Symbols.h:131
@@ +130,3 @@
+class ObjectFileDefined : public Defined {
+  friend SymbolBody;
+public:
----------------
ruiu wrote:
> Why do you need this?
To implement lazy getName and getDebugName.

================
Comment at: COFF/Symbols.h:163
@@ -136,1 +162,3 @@
+  uint64_t getRVA() { return (*Data)->getRVA() + Sym.getValue(); }
+
   bool isCOMDAT() { return IsCOMDAT; }
----------------
ruiu wrote:
> Remove blank line.
Sure.

================
Comment at: COFF/Symbols.h:188
@@ -165,1 +187,3 @@
 private:
+  friend SymbolBody;
+
----------------
ruiu wrote:
> Do we need this?
Yes, it's used to implement compare.

http://reviews.llvm.org/D10792

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/