For testing purposes, it is more efficient to use a pass which performs the Inline Cost Analysis and displays all the information about instructions. The functionality of the flag "-print-instruction-deltas" (which is renamed to "-print-inline-cost-instruction-annotations" for clarity) remains.
We need this pass because several tests, existing and upcoming, will require this functionality, but for now, it is limited only to debug builds.
Details
- Reviewers
apilipenko davidxl mtrofin fedor.sergeev
Diff Detail
Event Timeline
Could you split this into 2 changes:
- one that enables printing per-instruction inline cost analysis grouped together (i.e. the reason InstructionCostDetailMap is introduced)
- the other that introduces the new pass.
llvm/lib/Analysis/InlineCost.cpp | ||
---|---|---|
59 | I think 'Annotate' refers to adding metadata, is that the intent? Should the rewording happen when that's implemented, rather (for clarity) | |
409 | Consider InlineCostCallAnalyzer *const ICCA, to force the initialization of the pointer in the ctor. This helps maintainability - we won't risk ICCA bein uninitialized; it also captures design intent - ICCA, once set, isn't re-set (IIUC). | |
413 | drop virtual, add override, so the compiler would check for you that you're actually overriding the base class' emitInstructionAnnot, not just adding another virtual (easier to maintain code - if later there's a refactoring in the base class, for instance, it'd be caught) | |
741 | the move of the code in the file seems unrelated to this change - I'd propose doing it in a separate patch instead (if it's important to do). | |
2524 | This won't give you the Params used for inlining, it'll give you ad default Params, is that intended? |
Addressed @mtrofin 's comments:
- Before the change, we had CostAnnotationWriter that was used just to display the cost difference per instruction, hence the CostThresholdMap (which now became InstructionCostDetailMap) was in the printer itself. Now, the change changed that by putting the map into InlineCostCallAnalyzer to keep all the data in this class.
- The move of code was unnecessary, changed it
Oh - sorry, I saw you made changes for the comments on lines 420, 424 and 742, but didn't see anything on 59 and 2525, so I assumed you were looking to address those. Also, would it make sense to split off the pass in its own change (the other comment).
Could you also mark 'done' the addressed comments (it's easier to keep track for anyone looking at the CL)
Thanks!
All of the preparational patches were approved, but there was an issue with integration which is being resolved in D82205
I think 'Annotate' refers to adding metadata, is that the intent? Should the rewording happen when that's implemented, rather (for clarity)