Page MenuHomePhabricator

[LTO] Add ability to emit assembly to new LTO API
ClosedPublic

Authored by tobiasvk on Feb 2 2017, 3:32 PM.

Details

Summary

Add a field to LTO::Config, CGFileType, to select the file type to emit (object
or assembly). This is useful for testing and to implement -save-temps.

Diff Detail

Event Timeline

tobiasvk created this revision.Feb 2 2017, 3:32 PM
mehdi_amini edited edge metadata.Feb 2 2017, 3:37 PM

Can you clarify how one would implement -save-temps in a linker this way?

davide added a subscriber: davide.Feb 2 2017, 3:40 PM

ehm, we already have -save-temps, why do you need this? (see what the gold-plugin and lld do)

Can you clarify how one would implement -save-temps in a linker this way?

Set Conf.CGFileType to CGFT_Assembly, then run an external assembler on all outputs produced by LTO before linking. We leave the output files around and output a notice to the user about their location. This is what we've been doing with the legacy API, and we'd like to keep it as we move to the new API.

ehm, we already have -save-temps, why do you need this? (see what the gold-plugin and lld do)

It saves intermediate bitcode files (which is useful) but our customers also want to see assembly just like in the non-LTO flow.

Can you clarify how one would implement -save-temps in a linker this way?

Set Conf.CGFileType to CGFT_Assembly, then run an external assembler on all outputs produced by LTO before linking. We leave the output files around and output a notice to the user about their location. This is what we've been doing with the legacy API, and we'd like to keep it as we move to the new API.

That'd require to have a driver to run the external assembler in the linker, which isn't very common right? Otherwise the build system would need to know about this, which is also a non-trivial integration. I guess an assembler does not need much setup in terms of flags to pass, but still it is not clear to me if we'd be able to get the same binary in the end. That'd seem quite "fragile" to me.
However for the LLVM side of it, the patch seems non-intrusive enough that it is fine to me.

@pcc, any objection?

pcc edited edge metadata.Feb 2 2017, 4:13 PM

I would echo Mehdi's comments: I think I would be surprised if -save-temps changed the output at all (it sounds like it would cause the linker to use a different assembler). If I were implementing this feature, I think I would consider alternatives first, such as making the "use an external assembler" feature orthogonal to -save-temps.

However, supporting a "use an external assembler" feature at the API level does not seem too burdensome. Can you please make a separate test in LTO/Resolution though?

tobiasvk updated this revision to Diff 86977.Feb 3 2017, 9:07 AM

Move test to test/LTO/Resolution as per Peter's request.

Thank you all for your comments.

I agree about the need for an assembler 'driver' in the linker to make this
work and the potential fragility. This is what we have and it's worked fine on
our side with the previous LTO API. Note that clang's non-LTO -save-temps flow
calls the assembler separately (via clang -cc1as), too. It's perfectly doable
to ensure that the linker is called with the correct options to pass through to
the assembler when clang is used as the driver for LTO.

I like Rafael's idea of using the MC layer to emit both assembly and an object
at the same time with the same settings (if that's what you're proposing) - we
could use that for the non-LTO flow in clang too. I haven't checked how easy
that would be, but it's perhaps worth considering in the future.

Anyway, if there are no further objections, I'd like this to go in so the
legacy and resolution-based LTO APIs are feature-compatible in this respect.

This revision is now accepted and ready to land.Feb 15 2017, 11:16 AM
tobiasvk closed this revision.Feb 15 2017, 12:48 PM