Details
- Reviewers
davidxl
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 39679 Build 39716: arc lint + arc unit
Event Timeline
Can you split the patches into smaller ones -- at least 3 : ProfileSummary API changes, TTI API changes, and the rest (may need to be split too, but we will see).
Split the PSI changes into https://reviews.llvm.org/D67377.
It looks like the TTI part can't be separated due to dependencies and would pull in the whole thing:
The TTI files (TargetTransformInfo.h, TargetTransformInfoImpl.h, BasicTTIImpl.h, TargetTransformInfo.cpp) would pull in TargetLowering.h for the isSuitableForJumpTable change, which then would pull in SizeOpts.h/cpp for the llvm::shouldOptimizeForSize change. In addition, TargetLowering.h would also pull in SwitchLoweringUtils.cpp for the isSuitableForJumpTable change, which would pull in isSuitableForJumpTable.h for findJumpTables change, which would pull in SelectionDAGBuilder.cpp and IRTranslator.cpp for the findJumpTables changes,...etc.
Did you think of a different way to split the TTI part?
This patch should add test cases for each affected codegen passes.
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h | ||
---|---|---|
118 | Mark as unused args? |
llvm/include/llvm/Analysis/TargetTransformInfoImpl.h | ||
---|---|---|
118 | Done (note according to the comment for LLVM_ATTRIBUTE_UNUSED, a cast-to-void is preferred for unused variables.) |
SizeOpts and MachineSizeOpts changes can also be extracted into its own patch.
After this is done, the TTI change has one fewer dependency and we might figure out a way to isolate that part too..
llvm/lib/CodeGen/MachineSizeOpts.cpp | ||
---|---|---|
29 | I've looked at SizeOpts.h, SizeOpts.cpp, MachineSizeOpts.cpp files. I think we should refactorize the code using template to avoid duplicate logic. This should be done similarly to BlockFrequencyInfoImpl template <typename FuncT, typename BFIT> bool shouldOptimizeForSize(FuncT *F, ....) { } isColdBlock should probably refactorized in the similar way. |
The code can be broken down and contributed in the following order:
- SizeOpts related change
- TargetLowering change (isSuitableForJumpTable)
- TargetTransformation related changes -- depending on 1) and 2)
- SwitchLoweringUtils (findJumpTable depends on isSuitableForJumpTable in 2)
- the rest of the changes (can be further broken down per-pass).
I think the following allows us to split the TargetLowering/TTI/SwitchLoweringUtils part.
The TargetLowering (isSuitableForJumpTable) change pulls in BasicTTIImpl.h (getEstimatedNumberOfCaseClusters) and SwitchLoweringUtils.h/SwitchLoweringUtils.cpp (findJumpTables).
The getEstimatedNumberOfCaseClusters change pulls in TargetTransformInfo.h, TargetTransformInfo.cpp, TargetTransformInfoImpl.h, InlineCost.cpp.
The findJumpTables change pulls in IRTranslator.cpp and part of SelectionDAGBuilder.cpp.
Updated after dropping the TargetInstrInfo part (D69737).
Decreased the number of files by quite a few.
PTAL.
Mark as unused args?