This is an archive of the discontinued LLVM Phabricator instance.

[Darwin] Respect -fno-unroll-loops during LTO.
ClosedPublic

Authored by fhahn on Mar 27 2020, 4:06 AM.

Details

Summary

Currently -fno-unroll-loops is ignored when doing LTO on Darwin. This
patch adds a new -lto-no-unroll-loops option to the LTO code generator
and forwards it to the linker if -fno-unroll-loops is passed.

Diff Detail

Event Timeline

fhahn created this revision.Mar 27 2020, 4:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2020, 4:06 AM
thegameg accepted this revision.Mar 27 2020, 9:55 AM

LGTM, thanks!

This revision is now accepted and ready to land.Mar 27 2020, 9:55 AM
This revision was automatically updated to reflect the committed changes.
dexonsmith added a subscriber: Florian.EditedMar 27 2020, 6:03 PM

@fhahn, please revert, this isn't how we usually pass options in LTO.

If this is something we expect developers to use, it should be specifiable on a per-TU basis. The way we do this is by specifying it during compilation, attaching string-based function attributes, and checking that attribute at the beginning of the "unroll loop" pass to see whether to skip it for that function.

clang/lib/Driver/ToolChains/Darwin.cpp
545–550

I don't understand why we need driver support for this... is this something we expect users to do?

fhahn marked an inline comment as done.Mar 30 2020, 7:43 AM

Thanks for taking a look @dexonsmith!

@fhahn, please revert, this isn't how we usually pass options in LTO.

Reverted in 7899a111ea1160e2ae0aae42de37b14a0b75d71b.

It looks like there are similar options exposed by libLTO (-disable-inlining, -disable-gvn-loadpre, -disable-lto-vectorization). However those are not hooked up in the driver, presumably expecting the user to pass them to the linker through clang.

It seems like currently clang is not too consistent when it comes to handling arguments for LTO. Is there some documentation describing how various options should interact with LTO?

If this is something we expect developers to use, it should be specifiable on a per-TU basis. The way we do this is by specifying it during compilation, attaching string-based function attributes, and checking that attribute at the beginning of the "unroll loop" pass to see whether to skip it for that function.

Agreed, I think we should respect -fno-unroll-loops on a TU basis. I've put up D77058 to use the existing llvm.loop.unroll.disable metadata.

Is there any documentation on how TU level flags should interact with inlining across TU without those options? D77058 means that the loops in TUs compiled with -fno-unroll-loops won't be unrolled if they are inlined in functions in TUs without -fno-unroll-loops and loops from functions without -fno-unrolled-loops inlined into functions in TUs with -fno-unroll-loops will get unrolled. That is, -fno-unroll-loops will get applied exactly to the loops in the original TU, regardless where they are inlined. It is not applied to functions that get inlined from TUs without -fno-unroll-loops.

clang/lib/Driver/ToolChains/Darwin.cpp
545–550

Clang provides a -fno-unroll-loops option and allows users to specify it together with LTO. I think it is desirable for users to respect the option during LTO. For example, projects might have to disable unrolling, because their code is only correct assuming unrolling does not happen.

I think for the user it would be most convenient to disable unrolling during the clang linker invocation with LTO, together with an option to disable it per TU. I think that is similarly to how the mcpu option is handled during LTO with the gold plugin for example.

Only providing a way to disable it per TU should also be fine as well I think, but then Clang should at least warn if -fno-unroll-loops is passed for linking with LTO (and ignored).

Does that seem reasonable?