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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
