[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 10:36, Igor Laevsky <igor at azulsystems.com> wrote:
> 
> 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.

LGTM.

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