[PATCH] Emit comment for gc.relocate's showing base and derived pointers in human readable form
Igor Laevsky
igor at azulsystems.com
Thu Apr 30 10:36:54 PDT 2015
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);
> }
>
>
>> }
>>
>
More information about the llvm-commits
mailing list