[BUG] "Rotate Loop" pass kills "llvm.vectorizer.enable" metadata

[zinovy.nis]

Summary

Hi.

I faced that "llvm.vectorizer.enable" metadata used by Loop Vectorizer and set by OpenMP pragmas are killed after the "Rotate Loop" pass. The branch containing the metadata is replaced with another one, but without any metadata.
So "Loop Vectorizer" won't force vectorization of this loop.

I fixed it by storing the loop metadata in the begining of LoopRotate pass and then restoring it just before return from the pass.
I also added a test for it. It checks that LoopVectorize sees "llvm.vectorizer.enable" metadata.

[nadav]

Looking good!

[ABataev]

LGTM!

[atrick]

LGTM

[amusman]

What if the call to simplifyLoopLatch (at line 93 in lib/Transforms/Scalar/LoopRotation.cpp) removes metadata and subsequent rotateLoop at line 97 will not do anything, here is a small testcase for this. Is it possible to restore metadata here in the same way?

; RUN: opt < %s -O2 -force-vector-unroll=2 -force-vector-width=4 -debug-only=loop-vectorize -stats -S 2>&1 | FileCheck %s

; CHECK: LV: Loop hints: force=enabled

target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define i32 @foo(i32 %a) #0 {
entry:
  br label %loop_cond

loop_cond:                                        ; preds = %loop_inc, %entry
  %indx = phi i32 [ 1, %entry ], [ %inc, %loop_inc ]
  %cmp = icmp ne i32 %indx, %a
  br i1 %cmp, label %return, label %loop_inc

loop_inc:                                         ; preds = %loop_cond
  %inc = add i32 %indx, 1
  br label %loop_cond, !llvm.loop !1

return:                                           ; preds = %loop_cond
  ret i32 0
}

attributes #0 = { nounwind uwtable }

!1 = metadata !{metadata !1, metadata !2}
!2 = metadata !{metadata !"llvm.vectorizer.enable", i1 true}

[zinovy.nis]

My new patch fixes the issue kindly pointed by Alexander M.

Please re-review it again.

[amusman]

LGTM

[zinovy.nis]

Could you please commit this patch for me?

[ABataev]

Committed revision 206266.

[TylerNowicki]

Hi Zinovy,

I have a question about your test case. Did you manually add the

lib/Transforms/Scalar/LoopRotation.cpp

 Pass *llvm::createLoopRotatePass() { return new LoopRotate(); }
 
 /// Rotate Loop L as many times as possible. Return true if
 /// the loop is rotated at least once.
 bool LoopRotate::runOnLoop(Loop *L, LPPassManager &LPM) {
   if (skipOptnoneFunction(L))
     return false;
 
+  // Save the loop metadata.
+  MDNode *LoopMD = L->getLoopID();
+
   LI = &getAnalysis<LoopInfo>();
   TTI = &getAnalysis<TargetTransformInfo>();
 
   // Simplify the loop latch before attempting to rotate the header
   // upward. Rotation may not be needed if the loop tail can be folded into the
   // loop exit.
   bool SimplifiedLatch = simplifyLoopLatch(L);
 
   // One loop can be rotated multiple times.
   bool MadeChange = false;
   while (rotateLoop(L, SimplifiedLatch)) {
     MadeChange = true;
     SimplifiedLatch = false;
   }
+
+  // Restore the loop metadata.
+  // NB! We presume LoopRotation DOESN'T ADD its own metadata.
+  if ((MadeChange || SimplifiedLatch) && LoopMD)
+    L->setLoopID(LoopMD);
+
   return MadeChange;
 }
 
 /// RewriteUsesOfClonedInstructions - We just cloned the instructions from the
 /// old header into the preheader.  If there were uses of the values produced by
 /// these instruction that were outside of the loop, we have to insert PHI nodes
 /// to merge the two values.  Do this now.
 static void RewriteUsesOfClonedInstructions(BasicBlock *OrigHeader,

lib/Transforms/Vectorize/LoopVectorize.cpp

bool runOnFunction(Function &F) override {
     ColdEntryFreq = BlockFrequency(BFI->getEntryFreq()) * ColdProb;
 
     // If the target claims to have no vector registers don't attempt
     // vectorization.
     if (!TTI->getNumberOfRegisters(true))
       return false;
 
     if (DL == NULL) {
-      DEBUG(dbgs() << "LV: Not vectorizing: Missing data layout\n");
+      DEBUG(dbgs() << "\nLV: Not vectorizing " << F.getName()
+                   << ": Missing data layout\n");
       return false;
     }
 
     // Build up a worklist of inner-loops to vectorize. This is necessary as
     // the act of vectorizing or partially unrolling a loop creates new loops
     // and can invalidate iterators across the loops.
     SmallVector<Loop *, 8> Worklist;
 
     for (Loop *L : *LI)
       addInnerLoop(*L, Worklist);
 
     // Now walk the identified inner loops.
     bool Changed = false;
     while (!Worklist.empty())
       Changed |= processLoop(Worklist.pop_back_val());
 
     // Process each loop nest in the function.
     return Changed;
   }
 
   bool processLoop(Loop *L) {
     assert(L->empty() && "Only process inner loops.");
-    DEBUG(dbgs() << "LV: Checking a loop in \""
+    DEBUG(dbgs() << "\nLV: Checking a loop in \""
                  << L->getHeader()->getParent()->getName() << "\" from "
                  << getDebugLocString(L->getHeader()->getFirstNonPHIOrDbg())
                  << "\n");
 
     LoopVectorizeHints Hints(L, DisableUnrolling);
 
+    DEBUG(dbgs() << "LV: Loop hints:"
+                 << " force=" << (Hints.Force == 0
+                                      ? "disabled"
+                                      : (Hints.Force == 1 ? "enabled" : "?"))
+                 << " width=" << Hints.Width << " unroll=" << Hints.Unroll
+                 << "\n");
+
     if (Hints.Force == 0) {
       DEBUG(dbgs() << "LV: Not vectorizing: #pragma vectorize disable.\n");
       return false;
     }
 
     if (!AlwaysVectorize && Hints.Force != 1) {
       DEBUG(dbgs() << "LV: Not vectorizing: No #pragma vectorize enable.\n");
       return false;
bool processLoop(Loop *L) {
     // vector instructions?
     if (F->hasFnAttribute(Attribute::NoImplicitFloat)) {
       DEBUG(dbgs() << "LV: Can't vectorize when the NoImplicitFloat"
             "attribute is used.\n");
       return false;
     }
 
     // Select the optimal vectorization factor.
-    LoopVectorizationCostModel::VectorizationFactor VF;
-    VF = CM.selectVectorizationFactor(OptForSize, Hints.Width);
+    const LoopVectorizationCostModel::VectorizationFactor VF =
+                          CM.selectVectorizationFactor(OptForSize, Hints.Width);
     // Select the unroll factor.
-    unsigned UF = CM.selectUnrollFactor(OptForSize, Hints.Unroll, VF.Width,
+    const unsigned UF = CM.selectUnrollFactor(OptForSize, Hints.Unroll, VF.Width,
                                         VF.Cost);
 
     DEBUG(dbgs() << "LV: Found a vectorizable loop ("
                  << VF.Width << ") in "
                  << getDebugLocString(L->getHeader()->getFirstNonPHIOrDbg())
                  << '\n');
     DEBUG(dbgs() << "LV: Unroll Factor is " << UF << '\n');
 
for (unsigned i=2; i <= VF; i*=2) {
     DEBUG(dbgs() << "LV: Vector loop of width " << i << " costs: " <<
           (int)VectorCost << ".\n");
     if (VectorCost < Cost) {
       Cost = VectorCost;
       Width = i;
     }
   }
 
-  DEBUG(dbgs() << "LV: Selecting VF = : "<< Width << ".\n");
+  DEBUG(dbgs() << "LV: Selecting VF: "<< Width << ".\n");
   Factor.Width = Width;
   Factor.Cost = Width * Cost;
   return Factor;
 }
 
 unsigned LoopVectorizationCostModel::getWidestType() {
   unsigned MaxWidth = 8;
 

test/Transforms/LoopVectorize/vect.omp.persistence.ll

+; RUN: opt < %s -O2 -force-vector-unroll=2 -force-vector-width=4 -debug-only=loop-vectorize -stats -S 2>&1 | FileCheck %s
+
+; Loop from "rotated"
+; CHECK: LV: Loop hints: force=enabled
+; Loop from "nonrotated"
+; CHECK: LV: Loop hints: force=enabled
+; No more loops in the module
+; CHECK-NOT: LV: Loop hints: force=
+; In total only 1 loop should be rotated.
+; CHECK: 1 loop-rotate
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; See http://reviews.llvm.org/D3348 for details.
+
+;
+; Test #1
+;
+; Ensure that "llvm.vectorizer.enable" metadata was not lost prior to LoopVectorize pass.
+; In past LoopRotate was clearing that metadata.
+;
+; The source C code is:
+; void rotated(float *a, int size)
+; {
+;   int t = 0;
+;   #pragma omp simd
+;   for (int i = 0; i < size; ++i) {
+;     a[i] = a[i-5] * a[i+2];
+;     ++t;
+;   }
+;}
+
+define void @rotated(float* nocapture %a, i64 %size) {
+entry:
+  %cmp1 = icmp sgt i64 %size, 0
+  br i1 %cmp1, label %for.header, label %for.end
+
+for.header:
+  %indvars.iv = phi i64 [ %indvars.iv.next, %for.body ], [ 0, %entry ]
+  %cmp2 = icmp sgt i64 %indvars.iv, %size
+  br i1 %cmp2, label %for.end, label %for.body
+
+for.body:
+
+  %0 = add nsw i64 %indvars.iv, -5
+  %arrayidx = getelementptr inbounds float* %a, i64 %0
+  %1 = load float* %arrayidx, align 4, !llvm.mem.parallel_loop_access !1
+  %2 = add nsw i64 %indvars.iv, 2
+  %arrayidx2 = getelementptr inbounds float* %a, i64 %2
+  %3 = load float* %arrayidx2, align 4, !llvm.mem.parallel_loop_access !1
+  %mul = fmul float %1, %3
+  %arrayidx4 = getelementptr inbounds float* %a, i64 %indvars.iv
+  store float %mul, float* %arrayidx4, align 4, !llvm.mem.parallel_loop_access !1
+
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  br label %for.header, !llvm.loop !1
+
+for.end:
+  ret void
+}
+
+!1 = metadata !{metadata !1, metadata !2}
+!2 = metadata !{metadata !"llvm.vectorizer.enable", i1 true}
+
+;
+; Test #2
+;
+; Ensure that "llvm.vectorizer.enable" metadata was not lost even
+; if loop was not rotated (see http://reviews.llvm.org/D3348#comment-4).
+;
+define i32 @nonrotated(i32 %a) {
+entry:
+  br label %loop_cond
+loop_cond:
+  %indx = phi i32 [ 1, %entry ], [ %inc, %loop_inc ]
+  %cmp = icmp ne i32 %indx, %a
+  br i1 %cmp, label %return, label %loop_inc
+loop_inc:
+  %inc = add i32 %indx, 1
+  br label %loop_cond, !llvm.loop !3
+return:
+  ret i32 0
+}
+
+!3 = metadata !{metadata !3, metadata !4}
+!4 = metadata !{metadata !"llvm.vectorizer.enable", i1 true}