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

[Igor Laevsky]

Hi Duncan,

> 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?


It seems obviously profitable for every printout of the gc relocate. I.e without this comment in case of a large live sets it's almost impossible to understand to which pointers gc.relocate refers.

> 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);
>    }

You are right, this is cleaner. Please see the updated diff.

— Igor

On 30 Apr 2015, at 19:42, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:

> 
>> 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);
>    }
> 
> 
>> }
>> 
>