GCMetadataPrinter::finishAssembly not executed?

Hi,

After rebasing my local LLVM repo to ToT, I noticed that the
finishAssembly function is not executed and, thus, the stack map is not
printed at all.

Is this a known issue or I 'm doing something wrong?

I used a custom GCMetadataPrinter plugin but I reproduced this using the
builtin "ocaml" GC plugin and the attached file (actually, any simple ll
file that uses "ocaml" gc is fine).

Note: The beginAssembly function works great.

Best wishes,
yiannis

gctest.ll (1.1 KB)

Ping for this.

After rebasing my local LLVM repo to ToT, I noticed that the
finishAssembly function is not executed and, thus, the stack map is not
printed at all.

Is this a known issue or I 'm doing something wrong?

I used a custom GCMetadataPrinter plugin but I reproduced this using the
builtin "ocaml" GC plugin and the attached file (actually, any simple ll
file that uses "ocaml" gc is fine).

Note: The beginAssembly function works great.
   

I've tried adding two debug prints in lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp: one in beginAssembly and one in finishAssembly. It seems to me that finishAssembly is never called.

Has anyone tried anything similar? Testing this should be rather straightforward. :slight_smile:

Thanks!
yiannis

Sorry for the triple-posting! :blush:

After rebasing my local LLVM repo to ToT, I noticed that the
finishAssembly function is not executed and, thus, the stack map is not
printed at all.

Is this a known issue or I 'm doing something wrong?

I used a custom GCMetadataPrinter plugin but I reproduced this using the
builtin "ocaml" GC plugin and the attached file (actually, any simple ll
file that uses "ocaml" gc is fine).

Note: The beginAssembly function works great.

I've tried adding two debug prints in lib/CodeGen/AsmPrinter/OcamlGCPrinter.cpp: one in beginAssembly and one in finishAssembly. It seems to me that finishAssembly is never called.

I used git bisect and I think the commit that is responsible for this is:

> d1abec365aa89a8497d9b615ccb4b21c72da9447 is the first bad commit
> commit d1abec365aa89a8497d9b615ccb4b21c72da9447
> Author: Pedro Artigas <partigas@apple.com>
> Date: Wed Dec 5 17:12:22 2012 +0000
>
> - Added calls to doInitialization/doFinalization to immutable passes
> - fixed ordering of calls to doFinalization to be the reverse of the pass run order due to potential dependencies
> - fixed machine module info to operate in the doInitialization/doFinalization model, also fixes some FIXMEs
>
> reviewed by Evan Cheng <evan.cheng@apple.com>
>
> git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@169391 91177308-0d34-0410-b5e6-96231b3b80d8

Evan or Pedro, can you please take a look at this? :slight_smile:

Thanks!
yiannis

Hello Yiannis,

I believe what is going on is that there is an issue with the way that information is deleted (the CG information). 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?

Pedro

Hi Pedro (et al.),

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! :slight_smile: 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

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

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! :slight_smile:

- 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

GCMetadata.patch (2.23 KB)

Hello Yiannis,

Your patch seems fine. It is option #2 which I am glad to hear solves the issue. It also removes more lines than it adds, I like patches that fix issues and have that property!

Anyhow if you add some testing, as you suggest, you should be good to go.

Thanks

Pedro

Pedro, Yiannis,

What’s about the usage case, when LLVM is used as a library and the user implements its custom pass, which dump the code (implemented as a FunctionPass, but not as Printer)?

You also missed in your changes the declaration of llvm::createGCInfoDeleter() in include/llvm/CodeGen/Passes.h

-Dmitry.

HelloAll,

I am not an expert in CG but if the printer has to delete the info explicitly I believe any other client would have to do the same, I do not believe this has changed with Yiannis change.

Oh … true … there is that to delete too …

Pedro

Hi Dmitry,

What's about the usage case, when LLVM is used as a library and the user implements its custom pass, which dump the code (implemented as a FunctionPass, but not as Printer)?

Hm, I haven't thought of that... I'll take a better look and try to find a more suitable solution. Any suggestion is more than welcome! :slight_smile:

You also missed in your changes the declaration of llvm::createGCInfoDeleter() in include/llvm/CodeGen/Passes.h

Oops, sorry! I suppose I missed that! Pedro, will you take care of that (by just removing it) or should I sent a trivial fix in llvm-commits (where I don't have direct commit access)?

yiannis

Yiannis, Pedro,

I’m not an expert here. Taking closer look I see that it makes sense to do this cleanup job in doFinalization() of the pass, which requested GC info. So the current fix seems to be ok and all external clients will need to do cleanup directly doing the same jobs as Printer::doFinalization() does now.

By the way, I’ve just noticed that comments for GCModuleInfo::clear() also needs to be updated, as it mentions deleter pass.

-Dmitry.

Hi ,

Can someone commit the attatched (cleanup) patch for r175528?

Thanks,
yiannis

gcinfodelter-cleanup.patch (1.49 KB)

Yiannis,

  • /// clear - Resets the pass. The metadata deleter pass calls this.
  • /// clear - Resets the pass. The metadata printer pass calls this.

I would propose different text here:
/// clear - Resets the pass. Any pass, which uses GCModuleInfo(), should call it in doFinalization().

Does it make sense?

-Dmitry

Hi Dmitry,

gcinfodeleter-cleanup-v2.patch (1.54 KB)