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

[Ayke via Phabricator via llvm-commits]

aykevl added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119342