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

Wed Mar 2 10:17:50 PST 2022

tejohnson added inline comments.
Herald added a project: All.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119342