Page MenuHomePhabricator

[FuncSpec][NFC] Refactor internal structures.
ClosedPublic

Authored by labrinea on Mar 1 2022, 10:30 AM.

Details

Summary

Factored out part of https://reviews.llvm.org/D119880 as requested on the review.

Diff Detail

Event Timeline

labrinea created this revision.Mar 1 2022, 10:30 AM
labrinea requested review of this revision.Mar 1 2022, 10:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 10:30 AM

Looks like a good refactoring, but needs a clang-format, and have added one question inline.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
462–463

Was wondering if we could change SpecializationList to:

SmallVector<std::unique_ptr<SpecializationInfo>>;

so that we move and transfer ownership instead of copying things here.

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 1:44 AM
fhahn added a subscriber: fhahn.Mar 2 2022, 2:00 AM
fhahn added inline comments.
llvm/include/llvm/Transforms/Utils/SCCPSolver.h
154–160

Does the function modify Arg? If not, please either pass by value or as const reference.

labrinea added inline comments.Mar 2 2022, 5:43 AM
llvm/include/llvm/Transforms/Utils/SCCPSolver.h
154–160

Will do, thanks.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
462–463

Good remark, but will there really be a copy? I thought that the Return Value Optimization (copy elision) prevents that. Alternatively I could make the function void and pass the SpecializationList by reference on the argument list.

SjoerdMeijer added inline comments.Mar 2 2022, 7:26 AM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
462–463

My understanding is that this is an optimisation that may or may not be implemented. Looks like to be specified for c++17 though, but since we are using C++14 being explicit might be better.

labrinea updated this revision to Diff 412426.Mar 2 2022, 8:25 AM

Changes from last revision:

  • marked ArgInfo as const reference when passed to markArgInFuncSpecialization() and rewriteCallSites()
  • passed SpecializationList by reference to calculateGains() instead of returning by value
  • removed the IsPartial flag from isArgumentInteresting() and getPossibleConstants() as it's no longer used anywhere in the code
  • clang-formatted the code
SjoerdMeijer accepted this revision.Mar 3 2022, 1:51 AM

Thanks, looks like a good change to me.

This revision is now accepted and ready to land.Mar 3 2022, 1:51 AM
This revision was landed with ongoing or failed builds.Mar 3 2022, 5:09 AM
This revision was automatically updated to reflect the committed changes.