Loop unroller: set thresholds for optsize and minsize functions to zero

[hans]

Summary

Before r268509, Clang would disable the loop unroll pass when optimizing for size. That commit enabled it, to be able to support unroll pragmas in -Os builds. However, this regressed binary size in one of Chromium's DLLs with ~100 KB.

This restores the original behaviour of no unrolling at -Os, but doing it in LLVM instead of Clang makes more sense, and also allows the pragmas to keep working.

[mamai]

test/Transforms/LoopUnroll/unroll-opt-attribute.ll

7–10 ↗

(On Diff #56741)

This test should not need to be modified if the #pragma unroll still works in /Os... The -unroll-count=4 should simulate the unroll pragma, and therefor the loop should be unrolled even if the optsize attribute is there.

[rnk]

test/Transforms/LoopUnroll/unroll-opt-attribute.ll

7–10 ↗

(On Diff #56741)

Sounds like the LLVM pass is misbehaving then, and we should revert the clang side change to enable the loop unroller until this is addressed in LLVM.

[hans]

test/Transforms/LoopUnroll/unroll-opt-attribute.ll

7–10 ↗

(On Diff #56741)

I'm looking into it. I'm not sure the -unroll-count flag and the pragma do the same thing. Maybe this is easy to fix..

[hans]

test/Transforms/LoopUnroll/unroll-opt-attribute.ll

7–10 ↗

(On Diff #56741)

Yeah, -unroll-count does not simulate the pragma (is it supposed to?). It does set the count, but it doesn't modify the thresholds, whereas the pragma does:

if (PragmaCount > 0 ||
    ((PragmaFullUnroll || PragmaEnableUnroll) && TripCount != 0)) {                                                                    
  // If the loop has an unrolling pragma, we want to be more aggressive with                                                           
  // unrolling limits. Set thresholds to at least the PragmaTheshold value                                                             
  // which is larger than the default limits.                                                                                          
  if (UP.Threshold != NoThreshold)                                                                                                     
    UP.Threshold = std::max<unsigned>(UP.Threshold, PragmaUnrollThreshold);                                                            
  if (UP.PartialThreshold != NoThreshold)                                                                                              
    UP.PartialThreshold =
        std::max<unsigned>(UP.PartialThreshold, PragmaUnrollThreshold);                                                                
}

The pragmas are tested in test/Transforms/LoopUnroll/unroll-pragmas.ll I'll add a test there for the optsize case.

[hans]

Adding a test that makes sure the unroll pragma works at optsize.

Please take another look.

[mamai]

Ok then, it sounds good to me, at least for the test part. But I don't think I am the right person to accept this commit. I don't know why there was a threshold of 50 for optimize for size or why there was another threshold for partial unrolling. There might frontends other than clang that used it?

[mzolotukhin]

Hi,

In general I agree with what this patch is trying to fix, but wouldn't it be easier to just set PartialOptSizeThreshold and OptSizeThreshold to 0 in BasicTTI? These attributes are aimed exactly at what you are trying to solve, but we just happened to not initialize them with proper values.

Thanks,
Michael

[hans]

In D20115#426187, @mzolotukhin wrote:

In general I agree with what this patch is trying to fix, but wouldn't it be easier to just set PartialOptSizeThreshold and OptSizeThreshold to 0 in BasicTTI? These attributes are aimed exactly at what you are trying to solve, but we just happened to not initialize them with proper values.

Oops, I completely missed that these thresholds could be overridden by TTI. Thanks!

It seems the defaults are set here in gatherUnrollingPreferences() though, and then TTI can adjust them if it likes. I've updated the patch to simplify set the defaults to 0. Please take another look.

[mzolotukhin]

Thanks, this looks good to me. In terms of performance and code size/compile time this patch should only restore the behavior before r268509, right?

Michael

test/Transforms/LoopUnroll/unroll-opt-attribute.ll

110 ↗

(On Diff #56797)

Redundant space in the end.

[hans]

In D20115#426439, @mzolotukhin wrote:

Thanks, this looks good to me. In terms of performance and code size/compile time this patch should only restore the behavior before r268509, right?

For performance and code size, yes, this just restores the behaviour with Clang before r268509 (except for code with unroll pragmas). For compile time, we didn't use to have this pass in the pipeline at all before, so even with the threshold set to zero there is probably some cost.

test/Transforms/LoopUnroll/unroll-opt-attribute.ll

110 ↗

(On Diff #56797)

Will fix when committing, thanks.

llvm/trunk/lib/Transforms/Scalar/LoopUnrollPass.cpp

static TargetTransformInfo::UnrollingPreferences gatherUnrollingPreferences(
     Optional<bool> UserRuntime, unsigned PragmaCount, bool PragmaFullUnroll,
     bool PragmaEnableUnroll, unsigned TripCount) {
   TargetTransformInfo::UnrollingPreferences UP;
 
   // Set up the defaults
   UP.Threshold = 150;
   UP.PercentDynamicCostSavedThreshold = 20;
   UP.DynamicCostSavingsDiscount = 2000;
-  UP.OptSizeThreshold = 50;
+  UP.OptSizeThreshold = 0;
   UP.PartialThreshold = UP.Threshold;
-  UP.PartialOptSizeThreshold = UP.OptSizeThreshold;
+  UP.PartialOptSizeThreshold = 0;
   UP.Count = 0;
   UP.MaxCount = UINT_MAX;
   UP.FullUnrollMaxCount = UINT_MAX;
   UP.Partial = false;
   UP.Runtime = false;
   UP.AllowExpensiveTripCount = false;
 
   // Override with any target specific settings

llvm/trunk/test/Transforms/LoopUnroll/PowerPC/a2-unrolling.ll

 ; RUN: opt < %s -S -mtriple=powerpc64-unknown-linux-gnu -mcpu=a2 -loop-unroll | FileCheck %s -check-prefix=EPILOG
 ; RUN: opt < %s -S -mtriple=powerpc64-unknown-linux-gnu -mcpu=a2 -loop-unroll -unroll-runtime-epilog=false | FileCheck %s -check-prefix=PROLOG
-define void @unroll_opt_for_size() nounwind optsize {
-entry:
-  br label %loop
-
-loop:
-  %iv = phi i32 [ 0, %entry ], [ %inc, %loop ]
-  %inc = add i32 %iv, 1
-  %exitcnd = icmp uge i32 %inc, 1024
-  br i1 %exitcnd, label %exit, label %loop
-
-exit:
-  ret void
-}
-
-; EPILOG-LABEL: @unroll_opt_for_size
-; EPILOG:      add
-; EPILOG-NEXT: add
-; EPILOG-NEXT: add
-; EPILOG: icmp
-
-; PROLOG-LABEL: @unroll_opt_for_size
-; PROLOG:      add
-; PROLOG-NEXT: add
-; PROLOG-NEXT: add
-; PROLOG: icmp
 
 define i32 @test(i32* nocapture %a, i32 %n) nounwind uwtable readonly {
 entry:
   %cmp1 = icmp eq i32 %n, 0
   br i1 %cmp1, label %for.end, label %for.body
 
 for.body:                                         ; preds = %for.body, %entry
   %indvars.iv = phi i64 [ %indvars.iv.next, %for.body ], [ 0, %entry ]

llvm/trunk/test/Transforms/LoopUnroll/PowerPC/p7-unrolling.ll

 ; RUN: opt < %s -S -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 -loop-unroll | FileCheck %s
-define void @unroll_opt_for_size() nounwind optsize {
-entry:
-  br label %loop
-
-loop:
-  %iv = phi i32 [ 0, %entry ], [ %inc, %loop ]
-  %inc = add i32 %iv, 1
-  %exitcnd = icmp uge i32 %inc, 1024
-  br i1 %exitcnd, label %exit, label %loop
-
-exit:
-  ret void
-}
-
-; CHECK-LABEL: @unroll_opt_for_size
-; CHECK:      add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: icmp
-
 define void @unroll_default() nounwind {
 entry:
   br label %loop
 
 loop:
   %iv = phi i32 [ 0, %entry ], [ %inc, %loop ]
   %inc = add i32 %iv, 1
   %exitcnd = icmp uge i32 %inc, 1024

llvm/trunk/test/Transforms/LoopUnroll/partial-unroll-optsize.ll

-; RUN: opt < %s -S -loop-unroll -unroll-allow-partial | FileCheck %s
-; RUN: sed -e 's/optsize/minsize/' %s | opt -S -loop-unroll -unroll-allow-partial | FileCheck %s
-
-; Loop size = 3, when the function has the optsize attribute, the
-; OptSizeUnrollThreshold, i.e. 50, is used, hence the loop should be unrolled
-; by 32 times because (1 * 32) + 2 < 50 (whereas (1 * 64 + 2) is not).
-define void @unroll_opt_for_size() nounwind optsize {
-entry:
-  br label %loop
-
-loop:
-  %iv = phi i32 [ 0, %entry ], [ %inc, %loop ]
-  %inc = add i32 %iv, 1
-  %exitcnd = icmp uge i32 %inc, 1024
-  br i1 %exitcnd, label %exit, label %loop
-
-exit:
-  ret void
-}
-
-; CHECK:      add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: add
-; CHECK-NEXT: icmp

llvm/trunk/test/Transforms/LoopUnroll/unroll-opt-attribute.ll

 ; RUN: opt < %s -S -loop-unroll -unroll-count=4 | FileCheck -check-prefix=CHECK_COUNT4 %s
 ; RUN: opt < %s -S -loop-unroll | FileCheck -check-prefix=CHECK_NOCOUNT %s
 
 
 ;///////////////////// TEST 1 //////////////////////////////
 
-; This test shows that with optsize attribute, the loop is unrolled
-; according to the specified unroll factor.
+; This test shows that the loop is unrolled according to the specified
+; unroll factor.
 
-define void @Test1() nounwind optsize {
+define void @Test1() nounwind {
 entry:
   br label %loop
 
 loop:
   %iv = phi i32 [ 0, %entry ], [ %inc, %loop ]
   %inc = add i32 %iv, 1
   %exitcnd = icmp uge i32 %inc, 1024
   br i1 %exitcnd, label %exit, label %loop
 
 exit:
   ret void
 }
 
 ; CHECK_COUNT4-LABEL: @Test1
 ; CHECK_COUNT4:      phi
 ; CHECK_COUNT4-NEXT: add
 ; CHECK_COUNT4-NEXT: add
 ; CHECK_COUNT4-NEXT: add
 ; CHECK_COUNT4-NEXT: add
 ; CHECK_COUNT4-NEXT: icmp
 
 
 ;///////////////////// TEST 2 //////////////////////////////
 
-; This test shows that with minsize attribute, the loop is unrolled
-; according to the specified unroll factor.
-
-define void @Test2() nounwind minsize {
-entry:
-  br label %loop
-
-loop:
-  %iv = phi i32 [ 0, %entry ], [ %inc, %loop ]
-  %inc = add i32 %iv, 1
-  %exitcnd = icmp uge i32 %inc, 1024
-  br i1 %exitcnd, label %exit, label %loop
-
-exit:
-  ret void
-}
-
-; CHECK_COUNT4-LABEL: @Test2
-; CHECK_COUNT4:      phi
-; CHECK_COUNT4-NEXT: add
-; CHECK_COUNT4-NEXT: add
-; CHECK_COUNT4-NEXT: add
-; CHECK_COUNT4-NEXT: add
-; CHECK_COUNT4-NEXT: icmp
-
-
-;///////////////////// TEST 3 //////////////////////////////
-
 ; This test shows that with optnone attribute, the loop is not unrolled
 ; even if an unroll factor was specified.
 
-define void @Test3() nounwind optnone noinline {
+define void @Test2() nounwind optnone noinline {
 entry:
   br label %loop
 
 loop:
   %iv = phi i32 [ 0, %entry ], [ %inc, %loop ]
   %inc = add i32 %iv, 1
   %exitcnd = icmp uge i32 %inc, 1024
   br i1 %exitcnd, label %exit, label %loop
 
 exit:
   ret void
 }
 
-; CHECK_COUNT4-LABEL: @Test3
+; CHECK_COUNT4-LABEL: @Test2
 ; CHECK_COUNT4:      phi
 ; CHECK_COUNT4-NEXT: add
 ; CHECK_COUNT4-NEXT: icmp
 
 
-;///////////////////// TEST 4 //////////////////////////////
+;///////////////////// TEST 3 //////////////////////////////
 
-; This test shows that without any attribute, this loop is fully unrolled 
-; by default.
+; This test shows that this loop is fully unrolled by default.
 
 @tab = common global [24 x i32] zeroinitializer, align 4
 
-define i32 @Test4() {
+define i32 @Test3() {
 entry:
   br label %for.body
 
 for.body:                                         ; preds = %for.body, %entry
   %i.05 = phi i32 [ 0, %entry ], [ %inc, %for.body ]
   %arrayidx = getelementptr inbounds [24 x i32], [24 x i32]* @tab, i32 0, i32 %i.05
   store i32 %i.05, i32* %arrayidx, align 4
   %inc = add nuw nsw i32 %i.05, 1
   %exitcond = icmp eq i32 %inc, 24
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:                                          ; preds = %for.body
   ret i32 42
 }
 
-; CHECK_NOCOUNT-LABEL: @Test4
+; CHECK_NOCOUNT-LABEL: @Test3
 ; CHECK_NOCOUNT:      store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store

 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: store
 ; CHECK_NOCOUNT-NEXT: ret
 
 
-;///////////////////// TEST 5 //////////////////////////////
+;///////////////////// TEST 4 //////////////////////////////
 
-; This test shows that with optsize attribute, this loop is not unrolled 
-; by default.
+; This test shows that with optsize attribute, this loop is not unrolled.
 
-define i32 @Test5() optsize {
+define i32 @Test4() optsize {
 entry:
   br label %for.body
 
 for.body:                                         ; preds = %for.body, %entry
   %i.05 = phi i32 [ 0, %entry ], [ %inc, %for.body ]
   %arrayidx = getelementptr inbounds [24 x i32], [24 x i32]* @tab, i32 0, i32 %i.05
   store i32 %i.05, i32* %arrayidx, align 4
   %inc = add nuw nsw i32 %i.05, 1
   %exitcond = icmp eq i32 %inc, 24
   br i1 %exitcond, label %for.end, label %for.body
 
 for.end:                                          ; preds = %for.body
   ret i32 42
 }
 
-; CHECK_NOCOUNT-LABEL: @Test5
+; CHECK_NOCOUNT-LABEL: @Test4
 ; CHECK_NOCOUNT:      phi
 ; CHECK_NOCOUNT:      icmp

llvm/trunk/test/Transforms/LoopUnroll/unroll-pragmas.ll

for.body:                                         ; preds = %for.body, %entry
   br i1 %exitcond, label %for.end, label %for.body, !llvm.loop !3
 
 for.end:                                          ; preds = %for.body
   ret void
 }
 !3 = !{!3, !4}
 !4 = !{!"llvm.loop.unroll.full"}
 
+; #pragma clang loop unroll(full)
+; Loop should be fully unrolled, even for optsize.
+;
+; CHECK-LABEL: @loop64_with_full_optsize(
+; CHECK-NOT: br i1
+define void @loop64_with_full_optsize(i32* nocapture %a) optsize {
+entry:
+  br label %for.body
+
+for.body:                                         ; preds = %for.body, %entry
+  %indvars.iv = phi i64 [ 0, %entry ], [ %indvars.iv.next, %for.body ]
+  %arrayidx = getelementptr inbounds i32, i32* %a, i64 %indvars.iv
+  %0 = load i32, i32* %arrayidx, align 4
+  %inc = add nsw i32 %0, 1
+  store i32 %inc, i32* %arrayidx, align 4
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond = icmp eq i64 %indvars.iv.next, 64
+  br i1 %exitcond, label %for.end, label %for.body, !llvm.loop !3
+
+for.end:                                          ; preds = %for.body
+  ret void
+}
+
 ; #pragma clang loop unroll_count(4)
 ; Loop should be unrolled 4 times.
 ;
 ; CHECK-LABEL: @loop64_with_count4(
 ; CHECK: store i32
 ; CHECK: store i32
 ; CHECK: store i32
 ; CHECK: store i32