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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119342