[PATCH] IR: Add COMDATs to the IR

[David Majnemer]

================
Comment at: docs/LangRef.rst:664
@@ -659,1 +663,3 @@
+Additionally, the function can placed in a comdat if the target has the
+necessary support.
 
----------------
Rafael Ávila de Espíndola wrote:
> The repeated "if the target has the necessary supports" is a bit redundant. Just talk about the target differences in the comdat section.
OK, will do.

================
Comment at: docs/LangRef.rst:758
@@ +757,3 @@
+Note that not all targets support COMDATs and some may only support ``any`` as a
+selection kind.
+
----------------
Rafael Ávila de Espíndola wrote:
> We only have 3 file formats. It is probably OK to say that ELF only supports 'any' :-)
Sure.

================
Comment at: docs/LangRef.rst:771
@@ -717,1 +770,3 @@
+   }
+
 .. _namedmetadatastructure:
----------------
Rafael Ávila de Espíndola wrote:
> An important difference between COFF and ELF is that in COFF only one section can be in a COMDAT. In ELF, multiple sections can be in the same comdat.
> 
> I noticed that you left associative out of the above list. Is the intention to write code that look like ELF (multiple sections in on COMDAT) but at codegen we pick one to actually be in that comdat and make others associative with it?
> 
> It is probably a good idea to mention this, since the documentation gives the impression that adding COMDAT to a global makes sure that it will be in that COMDAT.
> 
> What are we doing about linkage? For COMDATs to work, we have to make sure their contents are not modified. For example, if a COMDAT has two linkonce_odr symbols, dropping only one can cause a llink failure.
> 
> One way would be to say that isDiscardableIfUnused symbols are not allowed, but we do want to allow private. Going this way would require spiting the "can be dropped" notion out of linkage. For example, linkonce would become "weak dropable" and a plain private would have to be kept. We do want to allow entire COMDATs to be dropped, so the comdat itself would probably need the dropable bit too.
> 
> A simpler but potentially brittle option is to just say that
> * A symbol in a comdat cannot be dropped individually
> * A comdat can be dropped if every symbol in it can be dropped.
> 
> And audit pretty much every call to isDiscardableIfUnused.
Consider:
  $v = comdat any
  @v = global i32 0, comdat $v
  @m = global i32 0, comdat $v

In the ELF case, `@v` will be selected as the COMDAT key. The .data.v and .data.m sections will both be in the COMDAT group.

In the COFF case, `@v`'s .data section will be the primary COMDAT section. `@m`'s .data section will be associated with it.

Because we have this behavior, we do not need an explicit associative COMDAT selection kind.

I'll work on fixing the discard problem.

================
Comment at: include/llvm/IR/Comdat.h:32
@@ +31,3 @@
+
+class Comdat : public Value, public ilist_node<Comdat> {
+public:
----------------
Rafael Ávila de Espíndola wrote:
> Why do you need it to be a Value? This says that a Comdat has a type, which seems wrong.
It is a `Value` so that it can participate in things like use lists.

As an example why this is useful:
If we want to drop a symbol in a COMDAT group, we need to prove we don't need any of the symbols in that group.  Iterating over users of the COMDAT is a straightforward way of doing this.

I could recreate this functionality just for COMDATs but it seemed like the right way to go.

================
Comment at: include/llvm/IR/Comdat.h:51
@@ +50,3 @@
+  /// Methods for support type inquiry through isa, cast, and dyn_cast:
+  static inline bool classof(const Value *V) {
+    return V->getValueID() == Value::ComdatVal;
----------------
Rafael Ávila de Espíndola wrote:
> What is this needed for?
There are places, like `ConvertValIDToValue`, where we get a `Comdat` from a `Value`.

================
Comment at: include/llvm/IR/Module.h:599
@@ +598,3 @@
+  comdat_iterator       comdat_begin()          { return ComdatList.begin(); }
+  const_comdat_iterator comdat_begin() const    { return ComdatList.begin(); }
+  comdat_iterator       comdat_end  ()          { return ComdatList.end();   }
----------------
Rafael Ávila de Espíndola wrote:
> Is this need? I looks a bit strange say that comdat is "part of a module". They are really a composite property of the globals.
A `Comdat` value has to live in *some* symbol table.  Considering the tight relationship between `Comdat` value names and `GlobalValue` names, it makes sense for the symbol table to be at `Module` scope.

================
Comment at: lib/AsmParser/LLParser.cpp:539
@@ +538,3 @@
+  case lltok::kw_exactmatch:   SK = Comdat::ExactMatch;   break;
+  case lltok::kw_largest:      SK = Comdat::Largest;      break;
+  case lltok::kw_newest:       SK = Comdat::Newest;       break;
----------------
Rafael Ávila de Espíndola wrote:
> Does clang-format agrees with having the break in the same line?
I matched the style in `ParseFnAttributeValuePairs`, I can reformat this to be in `clang-format` style if you wish.

================
Comment at: lib/AsmParser/LLParser.cpp:1161
@@ +1160,3 @@
+
+/// GetComdatVal - Get a Comdat with the specified name, creating a forward
+/// reference record if needed.
----------------
Rafael Ávila de Espíndola wrote:
> Don't repeat the name in the comment. Functions start with a lowecase letter.
I'll switch to `\brief`.

With regard to the naming style, I followed the [[ http://llvm.org/docs/CodingStandards.html#golden-rule | Golden Rule ]]

================
Comment at: lib/IR/AsmWriter.cpp:1315
@@ -1310,1 +1314,3 @@
 
+  // Output all comdats.
+  if (!M->comdat_empty()) Out << '\n';
----------------
Rafael Ávila de Espíndola wrote:
> This keeps unused comdats. It is probably better to keep track of which comdats are needed and output only those. This is similar to how an unused type is dropped.
> 
Sure, I can check to see if the `Comdat` has an empty use list.

================
Comment at: lib/IR/Globals.cpp:115
@@ +114,3 @@
+  if (isa<GlobalVariable>(this) || isa<GlobalAlias>(this)) {
+    // Comdats on declarations are semantically meaningless, ignore them.
+    if (isDeclaration())
----------------
Rafael Ávila de Espíndola wrote:
> Why not reject instead? This probably indicates a bug in the calling code, no?
It's annoying to avoid this from the `AsmParser`. I'll add a verifier check.

http://reviews.llvm.org/D4094