[PATCH] Emit comment for gc.relocate's showing base and derived pointers in human readable form

[Duncan P. N. Exon Smith]

> On 2015-Apr-30, at 04:05, Igor Laevsky <igor at azulsystems.com> wrote:
> 
> Duncan, could you please take a look at this changes? I am asking you because I am seeing a lot of submits from you to AsmParser lately. Please correct me if you are the wrong person to ask.
> 
> Philip, by using AnnotationWriter I can achieve same thing only for calls to which we explicitly pass AnnotationWriter. And there is no default AnnotationWriter. Because of this, for example, calls to Value->dump() will not emit intended comments. Also -print-after-all will  not emit them. This will reduce usability of this feature.
> 

There is some precedent for this kind of thing.  `MDNode`s used to have
a `WriteMDNodeComment()` that added comments to all the debug info nodes
(they were completely opaque).  And basic blocks always print out their
predecessor list.

I don't work on a compiler that uses GC, so this doesn't directly affect
me (and I don't really know how important the comment is).  Generally
the choice between comments in the AnnotationWriter and comments all the
time comes down to: is this essential for reading the IR?  You might
want to start an llvmdev thread to confirm that other GC-stakeholders
feel that the annotation is necessary... unless it's obviously so?

> Index: lib/IR/AsmWriter.cpp
> ===================================================================
> --- lib/IR/AsmWriter.cpp
> +++ lib/IR/AsmWriter.cpp
> @@ -31,6 +31,7 @@
>  #include "llvm/IR/LLVMContext.h"
>  #include "llvm/IR/Module.h"
>  #include "llvm/IR/Operator.h"
> +#include "llvm/IR/Statepoint.h"
>  #include "llvm/IR/TypeFinder.h"
>  #include "llvm/IR/UseListOrder.h"
>  #include "llvm/IR/ValueSymbolTable.h"
> @@ -2970,6 +2971,18 @@
>    I.getAllMetadata(InstMD);
>    printMetadataAttachments(InstMD, ", ");
>  
> +  // Print additional comment for gc.statepoint relocates in a form of
> +  // "; (<base pointer>, <derived pointer>)"
> +  if (isGCRelocate(&I)) {
> +    GCRelocateOperands GCOps(&I);
> +
> +    Out << " ; (";
> +    writeOperand(GCOps.basePtr(), false);
> +    Out << ", ";
> +    writeOperand(GCOps.derivedPtr(), false);
> +    Out << ")";
> +  }
> +
>    // Print a nice comment.
>    printInfoComment(I);

If this *is* the right spot, then it seems cleaner to move the body of
your `if` to a separate function, something like:

    // Always print GC statepoint comments.
    if (isGCRelocate(&I))
      printGCComment(I);
    // Print a nice comment.
    printInfoComment(I);

or possibly something like:

    void AssemblyWriter::printInfoComment(const Instruction &I) {
      // Print GC statepoint comments even without an AAW.
      if (isGCRelocate(&I))
        printGCComment(I);
      if (AAW)
        AAW->printInfoComment(I);
    }


>  }
>