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

[fhahn]

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.

[thegameg]

LGTM, thanks!

[dexonsmith]

@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

546–551

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

[fhahn]

Thanks for taking a look @dexonsmith!

In D76916#1947324, @dexonsmith wrote:

@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

546–551

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?

clang/lib/Driver/ToolChains/Darwin.cpp

void darwin::Linker::ConstructJob(Compilation &C, const JobAction &JA,
   // Setup statistics file output.
   SmallString<128> StatsFile =
       getStatsFileName(Args, Output, Inputs[0], getToolChain().getDriver());
   if (!StatsFile.empty()) {
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back(Args.MakeArgString("-lto-stats-file=" + StatsFile.str()));
   }
 
+  // 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"));
+  }
+
   // It seems that the 'e' option is completely ignored for dynamic executables
   // (the default), and with static executables, the last one wins, as expected.
   Args.AddAllArgs(CmdArgs, {options::OPT_d_Flag, options::OPT_s, options::OPT_t,
                             options::OPT_Z_Flag, options::OPT_u_Group,
                             options::OPT_e, options::OPT_r});
 
   // Forward -ObjC when either -ObjC or -ObjC++ is used, to force loading
   // members of static archive libraries which implement Objective-C classes or

clang/test/Driver/darwin-ld-lto-fno-unroll-loops.c

+// REQUIRES: system-darwin
+
+// RUN: mkdir -p %t/bin
+// RUN: mkdir -p %t/lib
+// RUN: touch %t/lib/libLTO.dylib
+
+// Check that ld gets "-lto-no-unroll-loops" when -fno-unroll-loops is passed.
+//
+// RUN: %clang -target x86_64-apple-darwin10 %s -fno-unroll-loops -flto=full -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=NOUNROLL %s
+
+// NOUNROLL:  "-mllvm" "-lto-no-unroll-loops"
+//
+// RUN: %clang -target x86_64-apple-darwin10 %s -flto=full -### 2>&1 | \
+// RUN:   FileCheck --check-prefix=UNROLL %s
+
+// UNROLL-NOT:  -lto-no-unroll-loops

llvm/lib/LTO/LTOCodeGenerator.cpp

cl::opt<std::string> RemarksFormat(
     cl::value_desc("format"), cl::init("yaml"));
 
 cl::opt<std::string> LTOStatsFile(
     "lto-stats-file",
     cl::desc("Save statistics to the specified file"),
     cl::Hidden);
 }
 
+cl::opt<bool> LTONoUnrollLoops("lto-no-unroll-loops",
+                               cl::desc("Disable unrolling during LTO."),
+                               cl::Hidden, cl::init(false));
+
 LTOCodeGenerator::LTOCodeGenerator(LLVMContext &Context)
     : Context(Context), MergedModule(new Module("ld-temp.o", Context)),
       TheLinker(new Linker(*MergedModule)) {
   Context.setDiscardValueNames(LTODiscardValueNames);
   Context.enableDebugTypeODRUniquing();
   initializeLTOPasses();
 }
 
bool LTOCodeGenerator::optimize(bool DisableVerify, bool DisableInline,
   // Add an appropriate DataLayout instance for this module...
   MergedModule->setDataLayout(TargetMach->createDataLayout());
 
   passes.add(
       createTargetTransformInfoWrapperPass(TargetMach->getTargetIRAnalysis()));
 
   Triple TargetTriple(TargetMach->getTargetTriple());
   PassManagerBuilder PMB;
+  PMB.DisableUnrollLoops = LTONoUnrollLoops;
   PMB.DisableGVNLoadPRE = DisableGVNLoadPRE;
   PMB.LoopVectorize = !DisableVectorization;
   PMB.SLPVectorize = !DisableVectorization;
   if (!DisableInline)
     PMB.Inliner = createFunctionInliningPass();
   PMB.LibraryInfo = new TargetLibraryInfoImpl(TargetTriple);
   if (Freestanding)
     PMB.LibraryInfo->disableAllFunctions();

llvm/test/tools/llvm-lto/fno-unroll-loops-option.ll

+; REQUIRES: asserts
+
+; RUN: llvm-as < %s > %t1.bc
+
+; Build with unrolling disabled (-lto-no-unroll-loops).
+; RUN: llvm-lto %t1.bc -o %t.nounroll.o -lto-no-unroll-loops --exported-symbol=foo -save-merged-module
+; RUN: llvm-dis  -o - %t.nounroll.o.merged.bc | FileCheck --check-prefix=NOUNROLL %s
+
+; NOUNROLL: br label %loop
+; NOUNROLL: br i1 %ec, label %exit, label %loop
+
+; Build with unrolling enabled (by not passing -lto-no-unroll-loops). All
+; branches should be gone.
+; RUN: llvm-lto %t1.bc -o %t.nounroll.o --exported-symbol=foo -save-merged-module
+; RUN: llvm-dis  -o - %t.nounroll.o.merged.bc | FileCheck --check-prefix=UNROLL %s
+
+; UNROLL-NOT: br
+
+define void @foo(i32* %ptr) {
+
+entry:
+  br label %loop
+
+loop:
+  %iv = phi i32 [ 0, %entry], [ %iv.next, %loop ]
+  %iv.ptr = getelementptr i32, i32* %ptr, i32 %iv
+  store i32 %iv, i32* %iv.ptr
+  %iv.next = add i32 %iv, 1
+  %ec = icmp eq i32 %iv.next, 10
+  br i1 %ec, label %exit, label %loop
+
+exit:
+  ret void
+}