Add llvm.loop.licm.disable metadata

[timcorringham]

Summary

For some targets the LICM pass can result in sub-optimal code in some
cases where it would be better not to run the pass, but it isn't
always possible to suppress the transformations heuristically.

Where the front-end has insight into such cases it is beneficial
to attach loop metadata to disable the pass - this change adds the
llvm.loop.licm.disable metadata to enable that.

[fhahn]

For some targets the LICM pass can result in sub-optimal code in some
cases where it would be better not to run the pass, but it isn't
always possible to suppress the transformations heuristically.

Can you provide more details on what targets those are and what the problems there are? How would the frontend decide? If it is target-specific, a TargetTransformInfo hook might be more suitable.

[timcorringham]

The target in this case is AMDGPU, and the problem is that transformations can create excessive register pressure ( but I don't believe this is necessarily unique to that target). The front-end has additional information available which can sometimes be used to identify such cases.

This change will only affect code when the metadata is used, so shouldn't have any adverse effect.

[hliao]

please update the doc on transformation metadata

[timcorringham]

Update LangRef doc

Added the new metadata to LangRef.rst

Updating D64557: Add llvm.loop.licm.disable metadata

[arsenm]

Can you post a real example? I would expect LICM to be used as a canonicalization pass, and then operations would be sunk as appropriate

[timcorringham]

Unfortunately I don't believe I have an example that is suitable for publishing. The problem is basically that the code motion is generally a good thing, but it can increase live ranges significantly pushing register pressure up such that performance is degraded, but that impact isn't apparent at the point at which the transformation is performed. Disabling the pass for certain loops is a low-impact pragmatic (but not ideal) solution.

[hfinkel]

In D64557#1580627, @timcorringham wrote:

Unfortunately I don't believe I have an example that is suitable for publishing. The problem is basically that the code motion is generally a good thing, but it can increase live ranges significantly pushing register pressure up such that performance is degraded, but that impact isn't apparent at the point at which the transformation is performed. Disabling the pass for certain loops is a low-impact pragmatic (but not ideal) solution.

Is this a rematerialization problem? We do hoist things sometimes which turn into multiple instructions and then we can't rematerialize during RA.

[Meinersbur]

lib/Transforms/Scalar/LICM.cpp

335

There is a hasDisableAllTransformsHint (llvm.loop.disable_nonforced) metadata that was intended to disable all (high-level) loop transformations. The use case is to ensure that no pass changes the loop before a user-defined transformation is applied (e.g. a loop is not unrolled before #pragma clang loop interleave(enable) is applied).

The prefix llvm.loop. indicates that LICM is also such a loop transformation and therefore disabled by llvm.loop.disable_nonforced, but I don't think we want that.

Maybe use llvm.licm.disable?

Also, there are several schemes of enable/disable metadata. The other one to disable a transformations is e.g. !{"llvm.loop.vectorize.enable", i1 0}. I am not sure which one is preferable; we have the mix already anyway.

[timcorringham]

Changed the metadata name to llvm.licm.disable

[timcorringham]

Some cases can be undone by rematerialization, but not all, and it can involve a lot of effort which increases compile time. The metadata is a pragmatic approach which helps in some cases.

lib/Transforms/Scalar/LICM.cpp

335

I have changed the metadata name to llvm.licm.disable.

As you note there is a variety of schemes, and I can't see a reason to choose one over another, so I'll leave it as it is.

[Meinersbur]

@fhahn @arsenm @hfinkel Has the motivation question been sufficiently addressed?

test/Transforms/LICM/pragma-licm-disable.ll

6

By convention, the RUN lines are the first in the file, even before comments.

30–38

Could you simplify the test? Remove branching within the loop, at most one PHI, one load (and maybe a store so the load result is not unused).

[timcorringham]

Simplified the LIT test

[timcorringham]

Does anyone have any remaining concerns with this as it is now?

[Meinersbur]

Please wait a day before committing in case there are still questions about the motivation.

[fhahn]

IMO it's fine to have this solution to address your immediate needs. I still think it would be good if we could get a reproducer, so we can address this properly in LICM.

include/llvm/Transforms/Utils/LoopUtils.h

 Optional<MDNode *>
 makeFollowupLoopID(MDNode *OrigLoopID, ArrayRef<StringRef> FollowupAttrs,
                    const char *InheritOptionsAttrsPrefix = "",
                    bool AlwaysNew = false);
 
 /// Look for the loop attribute that disables all transformation heuristic.
 bool hasDisableAllTransformsHint(const Loop *L);
 
+/// Look for the loop attribute that disables the LICM transformation heuristics.
+bool hasDisableLICMTransformsHint(const Loop *L);
+
 /// The mode sets how eager a transformation should be applied.
 enum TransformationMode {
   /// The pass can use heuristics to determine whether a transformation should
   /// be applied.
   TM_Unspecified,
 
   /// The transformation should be applied without considering a cost model.
   TM_Enable,

lib/Transforms/Scalar/LICM.cpp

 bool LoopInvariantCodeMotion::runOnLoop(
     Loop *L, AliasAnalysis *AA, LoopInfo *LI, DominatorTree *DT,
     TargetLibraryInfo *TLI, TargetTransformInfo *TTI, ScalarEvolution *SE,
     MemorySSA *MSSA, OptimizationRemarkEmitter *ORE, bool DeleteAST) {
   bool Changed = false;
 
   assert(L->isLCSSAForm(*DT) && "Loop is not in LCSSA form.");
 
+  // If this loop has metadata indicating that LICM is not to be performed then
+  // just exit.
+  if (hasDisableLICMTransformsHint(L)) {
+    return false;
+  }
+
   std::unique_ptr<AliasSetTracker> CurAST;
   std::unique_ptr<MemorySSAUpdater> MSSAU;
   bool NoOfMemAccTooLarge = false;
   unsigned LicmMssaOptCounter = 0;
 
   if (!MSSA) {
     LLVM_DEBUG(dbgs() << "LICM: Using Alias Set Tracker.\n");
     CurAST = collectAliasInfoForLoop(L, LI, AA);

lib/Transforms/Utils/LoopUtils.cpp

 #include "llvm/Transforms/Utils/BasicBlockUtils.h"
 
 using namespace llvm;
 using namespace llvm::PatternMatch;
 
 #define DEBUG_TYPE "loop-utils"
 
 static const char *LLVMLoopDisableNonforced = "llvm.loop.disable_nonforced";
+static const char *LLVMLoopDisableLICM = "llvm.loop.licm.disable";
 
 bool llvm::formDedicatedExitBlocks(Loop *L, DominatorTree *DT, LoopInfo *LI,
                                    MemorySSAUpdater *MSSAU,
                                    bool PreserveLCSSA) {
   bool Changed = false;
 
   // We re-use a vector for the in-loop predecesosrs.
   SmallVector<BasicBlock *, 4> InLoopPredecessors;
Optional<MDNode *> llvm::makeFollowupLoopID(
   FollowupLoopID->replaceOperandWith(0, FollowupLoopID);
   return FollowupLoopID;
 }
 
 bool llvm::hasDisableAllTransformsHint(const Loop *L) {
   return getBooleanLoopAttribute(L, LLVMLoopDisableNonforced);
 }
 
+bool llvm::hasDisableLICMTransformsHint(const Loop *L) {
+  return getBooleanLoopAttribute(L, LLVMLoopDisableLICM);
+}
+
 TransformationMode llvm::hasUnrollTransformation(Loop *L) {
   if (getBooleanLoopAttribute(L, "llvm.loop.unroll.disable"))
     return TM_SuppressedByUser;
 
   Optional<int> Count =
       getOptionalIntLoopAttribute(L, "llvm.loop.unroll.count");
   if (Count.hasValue())
     return Count.getValue() == 1 ? TM_SuppressedByUser : TM_ForcedByUser;

test/Transforms/LICM/pragma-licm-disable.ll

+; Check that the LICM pass does not operate on a loop which has the
+; llvm.loop.licm.disable metadata.
+; This test is simply a modified copy of constexpr.ll 
+
+; RUN: opt < %s -S -basicaa -licm | FileCheck %s
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc"
+
+@in = internal unnamed_addr global i32* null, align 8
+@out = internal unnamed_addr global i32* null, align 8
+
+; CHECK-LABEL: @licm_disable
+; CHECK: entry:
+; CHECK-NOT: load
+; CHECK: do.body:
+; CHECK: load i64, i64* bitcast (i32** @in to i64*)
+
+define i64 @licm_disable(i32 %N) {
+entry:
+  br label %do.body
+
+do.body:                                          ; preds = %l2, %entry
+  %i.0 = phi i32 [ 0, %entry ], [ %inc, %l2 ]
+  %total = phi i64 [ 0, %entry ], [ %next, %l2 ]
+  %c = icmp eq i32 %N, 6
+  br i1 %c, label %l1, label %do.body.l2_crit_edge
+
+do.body.l2_crit_edge:                             ; preds = %do.body
+  %inval.pre = load i32*, i32** @in, align 8
+  br label %l2
+
+l1:                                               ; preds = %do.body
+  %v1 = load i64, i64* bitcast (i32** @in to i64*), align 8
+  store i64 %v1, i64* bitcast (i32** @out to i64*), align 8
+  %0 = inttoptr i64 %v1 to i32*
+  br label %l2
+
+l2:                                               ; preds = %do.body.l2_crit_edge, %l1
+  %inval = phi i32* [ %inval.pre, %do.body.l2_crit_edge ], [ %0, %l1 ]
+  %int = ptrtoint i32* %inval to i64
+  %next = add i64 %total, %int
+  %inc = add nsw i32 %i.0, 1
+  %cmp = icmp slt i32 %inc, %N
+  br i1 %cmp, label %do.body, label %do.end, !llvm.loop !1
+
+do.end:                                           ; preds = %l2
+  ret i64 %total
+}
+!1 = !{!1, !2}
+!2 = !{!"llvm.loop.licm.disable"}
+