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

Wed Feb 9 11:57:44 PST 2022

aykevl added a comment.

In D119342#3308172 <https://reviews.llvm.org/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).

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:54
+  int Threshold =
+      (EnableHeaderDuplication && !L.getHeader()->getParent()->hasMinSize()) ||
+              hasVectorizeTransformation(&L) == TM_ForcedByUser
----------------
fhahn wrote:
> 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.
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`?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D119342/new/

https://reviews.llvm.org/D119342