Page MenuHomePhabricator

Convert OptimizationRemarkEmitter old emit() calls to new closure parameterized emit() calls
ClosedPublic

Authored by vivekvpandya on Sep 26 2017, 11:09 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

vivekvpandya created this revision.Sep 26 2017, 11:09 AM

changes Clang-formated!

anemet added inline comments.Oct 3 2017, 6:05 PM
lib/Analysis/ValueTracking.cpp
781 ↗(On Diff #117558)

Move this inside the lambda and then also no need for {}

lib/CodeGen/StackProtector.cpp
269–291 ↗(On Diff #117558)

auto RemarkBuilder = [&]() {

return OptimizationRemark(DEBUG_TYPE, "StackProtectorAllocaOrArray",
                                  &I)
            << "Stack protection applied to function "
            << ore::NV("Function", F)
            << " due to a call to alloca or use of a variable length array";

}

and then later just:

ORE.emit(RemarkBuilder);

lib/Transforms/IPO/Inliner.cpp
936–950 ↗(On Diff #117558)

Move the if inside the lambda?

lib/Transforms/IPO/SampleProfile.cpp
529–543 ↗(On Diff #117558)

Move 'if' inside the lambda

lib/Transforms/Instrumentation/PGOMemOPSizeOpt.cpp
386–393 ↗(On Diff #117558)

Move the 'using namespace' inside the lambda. You have more of this later.

lib/Transforms/Scalar/GVN.cpp
857 ↗(On Diff #117558)

The caller of this function already uses allowExtraAnalysis so no need to use the closure syntax here.

lib/Transforms/Vectorize/LoopVectorize.cpp
1251–1270 ↗(On Diff #117558)

Do this whole thing in the lambda.

5752–5754 ↗(On Diff #117558)

Again the point of the lambda to only conditionally construct the remark so please either build the remark inside or don't use the lambda syntax.

anemet edited edge metadata.Oct 3 2017, 6:06 PM

Also remember to add llvm-commit/cfe-commit when you create the review.

vivekvpandya marked 8 inline comments as done.

Updated patch to address @anemet 's comment.

anemet added a comment.Oct 9 2017, 8:54 AM

One more idea for improvement and then this is ready to go.

lib/Transforms/IPO/Inliner.cpp
935–945 ↗(On Diff #118165)

Can you actually factor this a bit further so that the common parts are done unconditionally. The reason why we use an insertion operator so that we can do such things.

lib/Transforms/IPO/SampleProfile.cpp
532–543 ↗(On Diff #118165)

Same thing here. Just build the common object and conditionally insert the discriminator.

vivekvpandya marked 2 inline comments as done.

updated

anemet accepted this revision.Oct 10 2017, 8:46 PM

LGTM. Thanks!

This revision is now accepted and ready to land.Oct 10 2017, 8:46 PM
This revision was automatically updated to reflect the committed changes.