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

Thu Feb 10 07:41:20 PST 2022

tejohnson added a comment.

> Is this the correct way to go, or do we still need a patch like D72404 <https://reviews.llvm.org/D72404> or D81223 <https://reviews.llvm.org/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.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119342