[PATCH] D76916: [Darwin] Respect -fno-unroll-loops during LTO.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

Thanks for taking a look @dexonsmith!

In D76916#1947324 <https://reviews.llvm.org/D76916#1947324>, @dexonsmith wrote:

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


Reverted in 7899a111ea1160e2ae0aae42de37b14a0b75d71b <https://reviews.llvm.org/rG7899a111ea1160e2ae0aae42de37b14a0b75d71b>.

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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.



================
Comment at: clang/lib/Driver/ToolChains/Darwin.cpp:546-551
+  // Forward -fno-unroll-loops to the linker in LTO.
+  if (Args.hasArg(options::OPT_fno_unroll_loops)) {
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-lto-no-unroll-loops"));
+  }
+
----------------
dexonsmith wrote:
> I don't understand why we need driver support for this... is this something we expect users to do?
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76916/new/

https://reviews.llvm.org/D76916