[PATCH] D136806: [Pipelines] Introduce SROA after (final, full) loop unrolling

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

In D136806#3931860 <https://reviews.llvm.org/D136806#3931860>, @spatel wrote:

> LGTM based on the timing, results, and alternatives discussed

Thank you for the review.

> There's some testing in progress according to previous comments, so best to wait for that to finish in case it turns up anything new.



================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:1119
+  if (IsFullLTO) {
+    FPM.addPass(SROAPass());
+  }
----------------
spatel wrote:
> Is there a reason to put this down here vs. tacking it on the end of the previous IsFullLTO block? 
> If LoopUnroll is the reason for adding SROA, then mention that specifically in the comment?
> 
> IIRC, all of the FullLTO predicates in this set of passes were questionable (see TODO comment above this function). They just accumulated because the code was duplicated and diverged over time.
> Is there a reason to put this down here vs. tacking it on the end of the previous IsFullLTO block?

I don't have any particular reason for this. Will change.

> If LoopUnroll is the reason for adding SROA, then mention that specifically in the comment?

Will do.

> IIRC, all of the FullLTO predicates in this set of passes were questionable (see TODO comment above this function). They just accumulated because the code was duplicated and diverged over time.

Yeah, i remember all that. This is indeed ugly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136806