llvm dev - Feb 2013 - [LLVMdev] GCMetadataPrinter::finishAssembly not executed?

Hi Pedro (et al.),

On 02/14/2013 11:43 PM, Pedro Artigas wrote:
> I believe what is going on is that there is an issue with the way that
information is deleted (the CG information).
Yeap, that's exactly the problem! :-) I noticed that the 
GCModuleInfo::iterator (line 931 in 
lib/CodeGen/AsmPrinter/AsmPrinter.cpp) is empty and, thus, 
GCMetadataPrinter::finishAssembly is never called.
> Right now there is a pass whose only job is to delete that information
(CGInfoDeleter) and that pass deletes the info before the AsmPrinter has a
chance to call the finishAssembly function. Right now the order that the
doFinalization is called on passes is the reverse order of doInitialization (and
that makes sense as if a pass depends on some other pass it has to initialize
after that pass and clean up before it) so the deleter pass would have to be
added in the pass order after the AsmPrinter pass as that pass clearly depends
on the Deleter pass (passes that have dependencies on other passes should
finalize before the pass it depends on does).
>
> I have a hunch that the very need for the Deleter pass is that the order of
deletion was somewhat undefined before and by having an specific pass one could
place the deletion wherever she/he sees fit. Now that the doFinalization order
is the reverse of the doInitialization one can probably just not have a deleter
and free the info inside the printer pass. The AsmPrinter depends on the Printer
pass so the new order should take care of initialization and finalization at the
right time.
>
> Does this make sense?
Hmm, I see what happens with the doInitialization/doFinalization and the 
pass dependencies. This makes sense to me.

One question though: does this mean that someone has to modify the 
GCMetadataPrinter instance to explicitly take care of the deletion of 
the metadata or changing the order of the Deleter/Printer passes should 
do the trick? If I got it right, this should happen automatically.

yiannis

-- 
Yiannis Tsiouris
Ph.D. student,
Software Engineering Laboratory,
National Technical University of Athens
WWW: http://www.softlab.ntua.gr/~gtsiour

Hello Yiannis,

I am not an expert on metadata or the CG information but one of the two has to
happen:

- Simply moving the deleter pass to a different spot
- Changing the doFinalization of another pass (Printer?) to do the deleter pass
job (this should work now because the doFinalization order is the reverse of the
doInitialization order)

Option 1 is a smaller change, option 2 will make the code cleaner and prevent
issues like this in the future, so it sounds like a better solution.

Thanks

Pedro

On Feb 15, 2013, at 4:25 AM, Yiannis Tsiouris <gtsiour at softlab.ntua.gr>
wrote:
> Hi Pedro (et al.),
> 
> On 02/14/2013 11:43 PM, Pedro Artigas wrote:
>> I believe what is going on is that there is an issue with the way that
information is deleted (the CG information).
> Yeap, that's exactly the problem! :-) I noticed that the
GCModuleInfo::iterator (line 931 in lib/CodeGen/AsmPrinter/AsmPrinter.cpp) is
empty and, thus, GCMetadataPrinter::finishAssembly is never called.
> 
>> Right now there is a pass whose only job is to delete that information
(CGInfoDeleter) and that pass deletes the info before the AsmPrinter has a
chance to call the finishAssembly function. Right now the order that the
doFinalization is called on passes is the reverse order of doInitialization (and
that makes sense as if a pass depends on some other pass it has to initialize
after that pass and clean up before it) so the deleter pass would have to be
added in the pass order after the AsmPrinter pass as that pass clearly depends
on the Deleter pass (passes that have dependencies on other passes should
finalize before the pass it depends on does).
>> 
>> I have a hunch that the very need for the Deleter pass is that the
order of deletion was somewhat undefined before and by having an specific pass
one could place the deletion wherever she/he sees fit. Now that the
doFinalization order is the reverse of the doInitialization one can probably
just not have a deleter and free the info inside the printer pass. The
AsmPrinter depends on the Printer pass so the new order should take care of
initialization and finalization at the right time.
>> 
>> Does this make sense?
> Hmm, I see what happens with the doInitialization/doFinalization and the
pass dependencies. This makes sense to me.
> 
> One question though: does this mean that someone has to modify the
GCMetadataPrinter instance to explicitly take care of the deletion of the
metadata or changing the order of the Deleter/Printer passes should do the
trick? If I got it right, this should happen automatically.
> 
> yiannis
> 
> -- 
> Yiannis Tsiouris
> Ph.D. student,
> Software Engineering Laboratory,
> National Technical University of Athens
> WWW: http://www.softlab.ntua.gr/~gtsiour
>

On 02/15/2013 07:13 PM, Pedro Artigas wrote:
> I am not an expert on metadata or the CG information but one of the two has
to happen:
I'm not an expert either but I'll give it a try! :-)
> - Simply moving the deleter pass to a different spot
> - Changing the doFinalization of another pass (Printer?) to do the deleter
pass job (this should work now because the doFinalization order is the reverse
of the doInitialization order)
> 
> Option 1 is a smaller change, option 2 will make the code cleaner and
prevent issues like this in the future, so it sounds like a better solution.
I'd vote for the second one! Does this mean we don't want GCInfoDeleter
any more? Because, IMO, we could do that by just moving the
Deleter::doFinalization functionality to Printer::doFinalization
(introduce it as this doesn't exist).

The attached patch seems to work. Take it as a prototype as it needs a
unittest just to be sure in the future!

Does this seem reasonable?

yianns

-- 
Yiannis Tsiouris
Ph.D. student,
Software Engineering Laboratory,
National Technical University of Athens
WWW: http://www.softlab.ntua.gr/~gtsiour
-------------- next part --------------
A non-text attachment was scrubbed...
Name: GCMetadata.patch
Type: text/x-diff
Size: 2199 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130216/0609b1f0/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 262 bytes
Desc: OpenPGP digital signature
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20130216/0609b1f0/attachment.sig>