[PATCH] Emit Clang version information into .comment section (LLVM's part of implementation) [PART 2]

[Katya Romanova]

  Hi Rafael,

  I'd like to get some clarification on what do you mean by "Why do we need to support both ways of having multiple .ident? It would be better to say (and check in the verifier) that each metadata node in a llvm.ident consists of just one string".

  Right now, we support multiple metadata nodes in llvm.ident, where each of these metadata nodes could have a list of strings:
  +!0 = metadata !{metadata !"clang version x.x"}
  +!1 = metadata !{metadata !"something else", metadata !"plus another"}
  +!2 = metadata !{i32 1}
  ----------------

  I believe you didn't like the fact that each of the metadata nodes could have a list of strings (i.e. you didn't like the following):
  +!1 = metadata !{metadata !"something else", metadata !"plus another"}

  Are you OK with having multiple metadata nodes in llvm.ident?
  +!0 = metadata !{metadata !"clang version x.x"}
  +!1 = metadata !{metadata !"something else"}
  +!2 = metadata !{i32 1}

  Or do you want only one metadata entry with just one string in it?
  +!0 = metadata !{metadata !"clang version x.x"}

  Is there any particular reason why would you want impose a limitation on the structure of metadata node in a llvm.ident? ln the future llvm.ident could be used for many different reasons, not only for storing compiler version information. For example, it could be used when  #ident directive is implemented or if we decide to emit compilation information (like a list of compilation options) to .comment section, etc.

  I don't see many justifications for limiting the structure of metadata node in llvm.ident. Why do you want to do that? The implementation of AsmPrinter::EMitModuleIdents() came out naturally with multiple strings in a metadata node. It will be more effort to limit this (i.e. allow only one string in a metadata node), because of all the checks and validations.

  I'm definitely against making it to be just one metadata node with just one string in it +!0 = metadata !{metadata !"clang version x.x"}. It will make it much harder to add something to llvm.ident if we want to. Hopefully, you don't want to impose this limitation.

  Also, could you please review and make comments on MC part of the patch? I have to switch back and forth between different non-compilable pieces of the patch now (since the patch is split). It's time consuming and error prone. It will be easier to make all the corrections, test and then send updated reviews for both parts.

  Thank you so much for reviewing!

  Katya.

  P.S. Is Clang's part of the patch OK?



  -----Original Message-----
  From: Rafael Ávila de Espíndola [mailto:rafael.espindola at gmail.com]
  Sent: Monday, September 30, 2013 2:35 PM
  To: Romanova, Katya
  Cc: llvm-commits at cs.uiuc.edu; rafael.espindola at gmail.com; justin.holewinski at gmail.com
  Subject: Re: [PATCH] Emit Clang version information into .comment section (LLVM's part of implementation) [PART 2]



  ================
  Comment at: include/llvm/CodeGen/AsmPrinter.h:490
  @@ -489,1 +489,3 @@
       void EmitLLVMUsedList(const ConstantArray *InitList);
  +    /// EmitModuleIdents - Emit llvm.ident metadata in an '.ident' directive.
  +    void EmitModuleIdents(Module &M);
  ----------------
  Please don't repeat the function name in the comment.

  ================
  Comment at: include/llvm/CodeGen/AsmPrinter.h:491
  @@ -490,1 +490,3 @@
  +    /// EmitModuleIdents - Emit llvm.ident metadata in an '.ident' directive.
  +    void EmitModuleIdents(Module &M);
       void EmitXXStructorList(const Constant *List, bool isCtor);
  ----------------
  emitModuleIdents would be the correct name according the the style guide. EmitModuleIdents is OK for now if you want.

  ================
  Comment at: test/CodeGen/X86/ident-metadata.ll:18
  @@ +17,3 @@
  +!0 = metadata !{metadata !"clang version x.x"}
  +!1 = metadata !{metadata !"something else", metadata !"plus another"}
  +!2 = metadata !{i32 1}
  ----------------
  Why do we need to support both ways of having multiple .ident? It would be better to say (and check in the verifier) that a each metadata node in a llvm.ident consists of just one string.



  http://llvm-reviews.chandlerc.com/D1729

http://llvm-reviews.chandlerc.com/D1729