[Clang] Add llvm.loop.unroll.disable to loops with -fno-unroll-loops.

[fhahn]

Summary

Currently Clang does not respect -fno-unroll-loops during LTO. During
D76916 it was suggested to respect -fno-unroll-loops on a TU basis.

This patch uses the existing llvm.loop.unroll.disable metadata to
disable loop unrolling explicitly for each loop in the TU if
unrolling is disabled. This should ensure that loops from TUs compiled
with -fno-unroll-loops are skipped by the unroller during LTO.

This also means that if a loop from a TU with -fno-unroll-loops
gets inlined into a TU without this option, the loop won't be
unrolled.

Due to the fact that some transforms might drop loop metadata, there
potentially are cases in which we still unroll loops from TUs with
-fno-unroll-loops. I think we should fix those issues rather than
introducing a function attribute to disable loop unrolling during LTO.
Improving the metadata handling will benefit other use cases, like
various loop pragmas, too. And it is an improvement to clang completely
ignoring -fno-unroll-loops during LTO.

If that direction looks good, we can use a similar approach to also
respect -fno-vectorize during LTO, at least for LoopVectorize.

In the future, this might also allow us to remove the UnrollLoops option
LLVM's PassManagerBuilder.

[tejohnson]

I think this is a good approach, rather than a per-function attribute, since as mentioned this will be preserved through inlining.
@dexonsmith, does that seem reasonable to you? I missed the original patch and agree with you that we don't want to fix this in LTO by passing the option through to LTO.

[dexonsmith]

In D77058#1950019, @tejohnson wrote:

I think this is a good approach, rather than a per-function attribute, since as mentioned this will be preserved through inlining.
@dexonsmith, does that seem reasonable to you? I missed the original patch and agree with you that we don't want to fix this in LTO by passing the option through to LTO.

SGTM!

[fhahn]

ping.

@tejohnson are you happy with this approach, given that it sounds good to @dexonsmith as well?

[tejohnson]

yep, LGTM

[Meinersbur]

Note that loop-metadata is best-effort only and may be forgotten in the optimization pipeline.

Do we also need an equivalent to -Xclang -disable-O0-optnone?

Personally, I don't like to the optnone approach: There have been many post on llvm-dev using clang -emit-llvm and being surprised that opt has no effect.

[fhahn]

In D77058#1964427, @Meinersbur wrote:

Note that loop-metadata is best-effort only and may be forgotten in the optimization pipeline.

Agreed, that can be a potential issue (I tried to note that in the description), but I think that's pretty much the same issue we have with the loop related pragmas. Ideally there would be even more incentive now to fix the offending transforms.

Do we also need an equivalent to -Xclang -disable-O0-optnone?

Personally, I don't like to the optnone approach: There have been many post on llvm-dev using clang -emit-llvm and being surprised that opt has no effect.

Ah yes, optnone is a common pitfall :(

IIUC we don't need a patch similar like this one for optnone, as it already gets added to the function attributes (for -O0) and has an option to disable adding it (-Xclang -disable-O0-optnone) on a per-TU basis.

[Meinersbur]

LGTM, since it continues current practice. optnone will always be the more annoying.

In D77058#1964714, @fhahn wrote:

IIUC we don't need a patch similar like this one for optnone, as it already gets added to the function attributes (for -O0) and has an option to disable adding it (-Xclang -disable-O0-optnone) on a per-TU basis.

My question was the other way around: Do we need something like -xclang -disable-fno-unroll-loops-metadata.

I documented for Polly how to get the IR for further processing. There is clang -Xclang -disable-O0-optnone and another is clang -O1 -Xclang -disable-llvm-passes. Both avoid the optnone attribute, but will yield different results with -fno-unroll-loops. Which is the 'correct' way?

clang/lib/CodeGen/CGLoopInfo.h

 class BasicBlock;
 class Instruction;
 class MDNode;
 } // end namespace llvm
 
 namespace clang {
 class Attr;
 class ASTContext;
+class CodeGenOptions;
 namespace CodeGen {
 
 /// Attributes that may be specified on loops.
 struct LoopAttributes {
   explicit LoopAttributes(bool IsParallel = false);
   void clear();
 
   /// Generate llvm.loop.parallel metadata for loads and stores.
public:
   /// Begin a new structured loop. The set of staged attributes will be
   /// applied to the loop and then cleared.
   void push(llvm::BasicBlock *Header, const llvm::DebugLoc &StartLoc,
             const llvm::DebugLoc &EndLoc);
 
   /// Begin a new structured loop. Stage attributes from the Attrs list.
   /// The staged attributes are applied to the loop and then cleared.
   void push(llvm::BasicBlock *Header, clang::ASTContext &Ctx,
+            const clang::CodeGenOptions &CGOpts,
             llvm::ArrayRef<const Attr *> Attrs, const llvm::DebugLoc &StartLoc,
             const llvm::DebugLoc &EndLoc);
 
   /// End the current loop.
   void pop();
 
   /// Return the top loop id metadata.
   llvm::MDNode *getCurLoopID() const { return getInfo().getLoopID(); }

clang/lib/CodeGen/CGLoopInfo.cpp

 //===---- CGLoopInfo.cpp - LLVM CodeGen for loop metadata -*- C++ -*-------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "CGLoopInfo.h"
 #include "clang/AST/ASTContext.h"
 #include "clang/AST/Attr.h"
 #include "clang/AST/Expr.h"
+#include "clang/Basic/CodeGenOptions.h"
 #include "llvm/IR/BasicBlock.h"
 #include "llvm/IR/CFG.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Metadata.h"
 using namespace clang::CodeGen;
 using namespace llvm;
void LoopInfoStack::push(BasicBlock *Header, const llvm::DebugLoc &StartLoc,
   Active.emplace_back(
       new LoopInfo(Header, StagedAttrs, StartLoc, EndLoc,
                    Active.empty() ? nullptr : Active.back().get()));
   // Clear the attributes so nested loops do not inherit them.
   StagedAttrs.clear();
 }
 
 void LoopInfoStack::push(BasicBlock *Header, clang::ASTContext &Ctx,
+                         const clang::CodeGenOptions &CGOpts,
                          ArrayRef<const clang::Attr *> Attrs,
                          const llvm::DebugLoc &StartLoc,
                          const llvm::DebugLoc &EndLoc) {
 
   // Identify loop hint attributes from Attrs.
   for (const auto *Attr : Attrs) {
     const LoopHintAttr *LH = dyn_cast<LoopHintAttr>(Attr);
     const OpenCLUnrollHintAttr *OpenCLHint =
case LoopHintAttr::Numeric:
       case LoopHintAttr::PipelineDisabled:
         llvm_unreachable("Options cannot be assigned a value.");
         break;
       }
       break;
     }
   }
 
+  if (CGOpts.OptimizationLevel > 0)
+    // Disable unrolling for the loop, if unrolling is disabled (via
+    // -fno-unroll-loops) and no pragmas override the decision.
+    if (!CGOpts.UnrollLoops &&
+        (StagedAttrs.UnrollEnable == LoopAttributes::Unspecified &&
+         StagedAttrs.UnrollCount == 0))
+      setUnrollState(LoopAttributes::Disable);
+
   /// Stage the attributes.
   push(Header, StartLoc, EndLoc);
 }
 
 void LoopInfoStack::pop() {
   assert(!Active.empty() && "No active loops to pop");
   Active.back()->finish();
   Active.pop_back();

clang/lib/CodeGen/CGStmt.cpp

 void CodeGenFunction::EmitWhileStmt(const WhileStmt &S,
                                     ArrayRef<const Attr *> WhileAttrs) {
   // Emit the header for the loop, which will also become
   // the continue target.
   JumpDest LoopHeader = getJumpDestInCurrentScope("while.cond");
   EmitBlock(LoopHeader.getBlock());
 
   const SourceRange &R = S.getSourceRange();
-  LoopStack.push(LoopHeader.getBlock(), CGM.getContext(), WhileAttrs,
-                 SourceLocToDebugLoc(R.getBegin()),
+  LoopStack.push(LoopHeader.getBlock(), CGM.getContext(), CGM.getCodeGenOpts(),
+                 WhileAttrs, SourceLocToDebugLoc(R.getBegin()),
                  SourceLocToDebugLoc(R.getEnd()));
 
   // Create an exit block for when the condition fails, which will
   // also become the break target.
   JumpDest LoopExit = getJumpDestInCurrentScope("while.end");
 
   // Store the blocks to use for break and continue.
   BreakContinueStack.push_back(BreakContinue(LoopExit, LoopHeader));
void CodeGenFunction::EmitDoStmt(const DoStmt &S,
   {
     RunCleanupsScope BodyScope(*this);
     EmitStmt(S.getBody());
   }
 
   EmitBlock(LoopCond.getBlock());
 
   const SourceRange &R = S.getSourceRange();
-  LoopStack.push(LoopBody, CGM.getContext(), DoAttrs,
+  LoopStack.push(LoopBody, CGM.getContext(), CGM.getCodeGenOpts(), DoAttrs,
                  SourceLocToDebugLoc(R.getBegin()),
                  SourceLocToDebugLoc(R.getEnd()));
 
   // C99 6.8.5.2: "The evaluation of the controlling expression takes place
   // after each execution of the loop body."
 
   // Evaluate the conditional in the while header.
   // C99 6.8.5p2/p4: The first substatement is executed if the expression
void CodeGenFunction::EmitForStmt(const ForStmt &S,
   // Start the loop with a block that tests the condition.
   // If there's an increment, the continue scope will be overwritten
   // later.
   JumpDest Continue = getJumpDestInCurrentScope("for.cond");
   llvm::BasicBlock *CondBlock = Continue.getBlock();
   EmitBlock(CondBlock);
 
   const SourceRange &R = S.getSourceRange();
-  LoopStack.push(CondBlock, CGM.getContext(), ForAttrs,
+  LoopStack.push(CondBlock, CGM.getContext(), CGM.getCodeGenOpts(), ForAttrs,
                  SourceLocToDebugLoc(R.getBegin()),
                  SourceLocToDebugLoc(R.getEnd()));
 
   // If the for loop doesn't have an increment we can just use the
   // condition as the continue block.  Otherwise we'll need to create
   // a block for it (in the current scope, i.e. in the scope of the
   // condition), and that we will become our continue block.
   if (S.getInc())
CodeGenFunction::EmitCXXForRangeStmt(const CXXForRangeStmt &S,
 
   // Start the loop with a block that tests the condition.
   // If there's an increment, the continue scope will be overwritten
   // later.
   llvm::BasicBlock *CondBlock = createBasicBlock("for.cond");
   EmitBlock(CondBlock);
 
   const SourceRange &R = S.getSourceRange();
-  LoopStack.push(CondBlock, CGM.getContext(), ForAttrs,
+  LoopStack.push(CondBlock, CGM.getContext(), CGM.getCodeGenOpts(), ForAttrs,
                  SourceLocToDebugLoc(R.getBegin()),
                  SourceLocToDebugLoc(R.getEnd()));
 
   // If there are any cleanups between here and the loop-exit scope,
   // create a block to stage a loop exit along.
   llvm::BasicBlock *ExitBlock = LoopExit.getBlock();
   if (ForScope.requiresCleanups())
     ExitBlock = createBasicBlock("for.cond.cleanup");

clang/test/CodeGenCXX/fno-unroll-loops-metadata.cpp

+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s -O0 -disable-llvm-optzns -fno-unroll-loops | FileCheck --check-prefix=NO_UNROLL_MD %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s -O1 -disable-llvm-optzns -fno-unroll-loops | FileCheck --check-prefix=UNROLL_DISABLED_MD %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s -O2 -disable-llvm-optzns -fno-unroll-loops | FileCheck --check-prefix=UNROLL_DISABLED_MD %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s -O3 -disable-llvm-optzns -fno-unroll-loops | FileCheck --check-prefix=UNROLL_DISABLED_MD %s
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s -O3 -disable-llvm-optzns | FileCheck --check-prefix=NO_UNROLL_MD %s
+
+// NO_UNROLL_MD-NOT: llvm.loop
+
+// Verify unroll.disable metadata is added to while loop with -fno-unroll-loops
+// and optlevel > 0.
+void while_test(int *List, int Length) {
+  // UNROLL_DISABLED_MD: define {{.*}} @_Z10while_test
+  int i = 0;
+
+  while (i < Length) {
+    // UNROLL_DISABLED_MD: br label {{.*}}, !llvm.loop ![[LOOP_1:.*]]
+    List[i] = i * 2;
+    i++;
+  }
+}
+
+// Verify unroll.disable metadata is added to do-while loop with
+// -fno-unroll-loops and optlevel > 0.
+void do_test(int *List, int Length) {
+  // UNROLL_DISABLED_MD: define {{.*}} @_Z7do_test
+  int i = 0;
+
+  do {
+    // UNROLL_DISABLED_MD: br i1 {{.*}}, label {{.*}}, label {{.*}}, !llvm.loop ![[LOOP_2:.*]]
+    List[i] = i * 2;
+    i++;
+  } while (i < Length);
+}
+
+// Verify unroll.disable metadata is added to while loop with -fno-unroll-loops
+// and optlevel > 0.
+void for_test(int *List, int Length) {
+  // UNROLL_DISABLED_MD: define {{.*}} @_Z8for_test
+  for (int i = 0; i < Length; i++) {
+    // UNROLL_DISABLED_MD: br label {{.*}}, !llvm.loop ![[LOOP_3:.*]]
+    List[i] = i * 2;
+  }
+}
+
+// UNROLL_DISABLED_MD: ![[LOOP_1]] = distinct !{![[LOOP_1]], ![[UNROLL_DISABLE:.*]]}
+// UNROLL_DISABLED_MD: ![[UNROLL_DISABLE]] = !{!"llvm.loop.unroll.disable"}
+// UNROLL_DISABLED_MD: ![[LOOP_2]] = distinct !{![[LOOP_2:.*]], ![[UNROLL_DISABLE:.*]]}
+// UNROLL_DISABLED_MD: ![[LOOP_3]] = distinct !{![[LOOP_3]], ![[UNROLL_DISABLE:.*]]}

clang/test/CodeGenCXX/pragma-unroll.cpp

 // RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - %s | FileCheck %s
 
+// Check that passing -fno-unroll-loops does not impact the decision made using pragmas.
+// RUN: %clang_cc1 -triple x86_64-apple-darwin -std=c++11 -emit-llvm -o - -O1 -disable-llvm-optzns -fno-unroll-loops %s | FileCheck %s
+
 // Verify while loop is recognized after unroll pragma.
 void while_test(int *List, int Length) {
   // CHECK: define {{.*}} @_Z10while_test
   int i = 0;
 
 #pragma unroll
   while (i < Length) {
     // CHECK: br label {{.*}}, !llvm.loop ![[LOOP_1:.*]]