[LoopRotate] Don't rotate loops when the minsize attribute is present

[aykevl]

Summary

The main use for this patch is LTO. It is not (yet?) possible to set the size level (-Os, -Oz) in the linker, which means loops are still rotated even if -Oz is specified on the command line. Therefore, look at the function attribute instead of at the size level to determine whether to rotate loops for a given function.

For related discussion, see D72404.

[fhahn]

Should we do this when the optsize function attribute is set, not just when the minsize function attribute is set?

I don't think so. Disabling rotation is a heavy hammer, as it will disable a lot of other loop optimizations implicitly due to most transforms requiring rotated form.

llvm/lib/Transforms/Scalar/LoopRotation.cpp

54

I think we we add minsize handling here we should also drop the EnableHeaderDuplication flag set when instantiating the pass. This is more in line with what most other passes do.

[aykevl]

In D119342#3308172, @fhahn wrote:

Should we do this when the optsize function attribute is set, not just when the minsize function attribute is set?

I don't think so. Disabling rotation is a heavy hammer, as it will disable a lot of other loop optimizations implicitly due to most transforms requiring rotated form.

Ok, sounds reasonable. After checking again this is indeed what happens in the default pass pipeline (old PM and new PM).
(I think it might be reasonable to reduce the rotation threshold in -Os but that's a separate issue).

llvm/lib/Transforms/Scalar/LoopRotation.cpp

54

Sounds like a good simplification. However, there is a case where this would result in a difference in behavior: buildO1FunctionSimplificationPipeline unconditionally disables header duplication.

LPM1.addPass(LoopRotatePass(/* Disable header duplication */ true,
                            isLTOPreLink(Phase)));

So, should EnableHeaderDuplication really be removed, if this means -O1 will become a bit more like -O2?

[tejohnson]

Is this the correct way to go, or do we still need a patch like D72404 or D81223?

IMO this is preferable. As much as possible the opt passes should be honoring function attributes. It allows them to work seamlessly with LTO, and it also allows correct handling of LTO linking of functions built with different flags. There's still some legacy handling that uses flags passed down from the driver, but we should be migrating to attributes as much as possible.

llvm/lib/Transforms/Scalar/LoopRotation.cpp

54

I guess the question this raises is why we don't have a function attribute for -O1 as we do for -O0/-Os/-Oz. Probably this field should be left until such attribute is added, or the change is evaluated at -O1.

[aykevl]

Ok, I've updated this patch to remove EnableHeaderDuplication.

Note that instead of duplicating headers at -O1, we might also remove LoopRotate at -O1 altogether? This would make sense to me, as I think vectorization shouldn't happen at -O1 anyway and -O1 generally only enables relatively simple transformations.

[aykevl]

Ping?

[tejohnson]

llvm/lib/Transforms/Scalar/LoopRotation.cpp

54

I don't think the removal of EnableHeaderDuplication should happen in this patch, which is about minsize. I would suggest splitting that change into a separate follow on patch.

[aykevl]

llvm/lib/Transforms/Scalar/LoopRotation.cpp

54

@tejohnson so you're saying I should revert this patch back to https://reviews.llvm.org/D119342?vs=on&id=407156? I removed EnableHeaderDuplication at the request of @fhahn.

[fhahn]

llvm/lib/Transforms/Scalar/LoopRotation.cpp

54

Sounds like a good simplification. However, there is a case where this would result in a difference in behavior: buildO1FunctionSimplificationPipeline unconditionally disables header duplication.

Oh right, that's unfortunate! Given that, it's probably better to keep EnableHeaderDuplication for now, as removing it is more complicated.

[aykevl]

• revert EnableHeaderDuplication removal

[aykevl]

@fhahn @tejohnson I've changed the patch. This one is now just for minsize.

[fhahn]

IIUC this should make LTO loop rotate behavior match the behavior when using -Oz in non-LTO, so this seems fine. Just a question about the test case. Also, it would be good to upload the diff with full context.

llvm/test/Transforms/LoopRotate/minsize-disable.ll

4

Does this test require running the full O2 pipeline? Usually we try to only run isolated passes if possible.

[aykevl]

Rebased the patch and updated the test case.

IIUC this should make LTO loop rotate behavior match the behavior when using -Oz in non-LTO, so this seems fine.

That's the intention.

Also, it would be good to upload the diff with full context.

Fixed. I sometimes forget it.

[aykevl]

llvm/test/Transforms/LoopRotate/minsize-disable.ll

4

I basically just copied oz-disable.ll but yeah in this case only the looprotate pass is relevant. I have updated the patch accordingly.

[aykevl]

Ping?

[aykevl]

ping?

[tejohnson]

lgtm

llvm/lib/Transforms/Scalar/LoopRotation.cpp

: EnableHeaderDuplication(EnableHeaderDuplication),
       PrepareForLTO(PrepareForLTO) {}
 
 PreservedAnalyses LoopRotatePass::run(Loop &L, LoopAnalysisManager &AM,
                                       LoopStandardAnalysisResults &AR,
                                       LPMUpdater &) {
   // Vectorization requires loop-rotation. Use default threshold for loops the
   // user explicitly marked for vectorization, even when header duplication is
   // disabled.
-  int Threshold = EnableHeaderDuplication ||
-                          hasVectorizeTransformation(&L) == TM_ForcedByUser
-                      ? DefaultRotationThreshold
-                      : 0;
+  int Threshold =
+      (EnableHeaderDuplication && !L.getHeader()->getParent()->hasMinSize()) ||
   const DataLayout &DL = L.getHeader()->getModule()->getDataLayout();
   const SimplifyQuery SQ = getBestSimplifyQuery(AR, DL);
 
   Optional<MemorySSAUpdater> MSSAU;
   if (AR.MSSA)
     MSSAU = MemorySSAUpdater(AR.MSSA);
   bool Changed =
       LoopRotation(&L, &AR.LI, &AR.TTI, &AR.AC, &AR.DT, &AR.SE,
bool runOnLoop(Loop *L, LPPassManager &LPM) override {
     auto *MSSAA = getAnalysisIfAvailable<MemorySSAWrapperPass>();
     if (MSSAA)
       MSSAU = MemorySSAUpdater(&MSSAA->getMSSA());
     // Vectorization requires loop-rotation. Use default threshold for loops the
     // user explicitly marked for vectorization, even when header duplication is
     // disabled.
     int Threshold = hasVectorizeTransformation(L) == TM_ForcedByUser
                         ? DefaultRotationThreshold
-                        : MaxHeaderSize;
+                        : (F.hasMinSize() ? 0 : MaxHeaderSize);
 
     return LoopRotation(L, LI, TTI, AC, &DT, &SE,
                         MSSAU.hasValue() ? MSSAU.getPointer() : nullptr, SQ,
                         false, Threshold, false,
                         PrepareForLTO || PrepareForLTOOption);
   }
 };
 } // end namespace

llvm/test/Transforms/LoopRotate/minsize-disable.ll

+; REQUIRES: asserts
+; RUN: opt < %s -S -O2 -debug -debug-only=loop-rotate 2>&1 | FileCheck %s
+; RUN: opt < %s -S -passes='default<O2>' -debug -debug-only=loop-rotate 2>&1 | FileCheck %s
+
+; Loop should not be rotated for functions with the minsize attribute.
+; This is mostly useful for LTO which doesn't (yet) understand -Oz.
+; CHECK: LoopRotation: NOT rotating - contains 2 instructions, which is more
+
+@e = global i32 10
+
+declare void @use(i32)
+
+; Function attrs: minsize optsize
+define void @test() #0 {
+entry:
+  %end = load i32, i32* @e
+  br label %loop
+
+loop:
+  %n.phi = phi i32 [ %n, %loop.fin ], [ 0, %entry ]
+  %cond = icmp eq i32 %n.phi, %end
+  br i1 %cond, label %exit, label %loop.fin
+
+loop.fin:
+  %n = add i32 %n.phi, 1
+  call void @use(i32 %n)
+  br label %loop
+
+exit:
+  ret void
+}
+
+attributes #0 = { minsize optsize }