This is an archive of the discontinued LLVM Phabricator instance.

[PM] BitcodeWriterPass should derive from PassInfoMixin
ClosedPublic

Authored by tejohnson on Aug 12 2016, 1:51 PM.

Details

Summary

The BitcodeWriterPass was ported a couple years ago, and predates the
PassInfoMixin. Make BitcodeWriterPass from that base class.

Should BitcodeWriterPass be added to the PassRegistry.def file? It seems
like that is only for passes that can be added arbitrarily, e.g. via the
-passes flag to the opt tool. Whereas the bitcode writer is added
specially based on the output type (and requires an output stream and
other parameters). For now I have left it out of the PassRegistry, but
let me know if it should go there.

Finally, I was considering an NFC change of the legacy WriteBitcodePass
to BitcodeWriterLegacyPass to make its usage clearer and more consistent
with other legacy passes. WDYT?

Diff Detail

Repository
rL LLVM

Event Timeline

tejohnson updated this revision to Diff 67907.Aug 12 2016, 1:51 PM
tejohnson retitled this revision from to [PM] BitcodeWriterPass should derive from PassInfoMixin.
tejohnson updated this object.
tejohnson added a reviewer: chandlerc.
tejohnson added a subscriber: llvm-commits.
mehdi_amini added inline comments.
include/llvm/Bitcode/BitcodeWriterPass.h
72 ↗(On Diff #67907)

The only point of the Mixin right now is to avoid this method, so you should probably remove it as well.

tejohnson updated this revision to Diff 67913.Aug 12 2016, 2:34 PM

Remove name() method, which is not needed when deriving from Mixin

mehdi_amini accepted this revision.Aug 12 2016, 2:38 PM
mehdi_amini added a reviewer: mehdi_amini.

LGTM.
I'm not sure about the registration, I think right now it is only to be able to schedule pass from a string? This shouldn't apply this this pass.

TBH, I even wonder if having the bitcode writer as a pass is useful at all? (It does not really hurt keeping it though...)

This revision is now accepted and ready to land.Aug 12 2016, 2:38 PM

LGTM.
I'm not sure about the registration, I think right now it is only to be able to schedule pass from a string? This shouldn't apply this this pass.

Yeah, that's my thought. Will commit as-is, if others think it needs to be registered that can be added as a follow-on.

This revision was automatically updated to reflect the committed changes.