Page MenuHomePhabricator

Refactor indirect call promotion profitability analysis (NFC)
ClosedPublic

Authored by tejohnson on Jul 8 2016, 5:13 PM.

Details

Summary

Refactored the profitability analysis out of the IC promotion pass and
into lib/Analysis so that it can be accessed by the summary index
builder in a follow-on patch to enable IC promotion in ThinLTO (D21932).

Diff Detail

Event Timeline

tejohnson updated this revision to Diff 63358.Jul 8 2016, 5:13 PM
tejohnson retitled this revision from to Refactor indirect call promotion profitability analysis (NFC).
tejohnson updated this object.
tejohnson added reviewers: davidxl, xur.
tejohnson added a subscriber: llvm-commits.
davidxl edited edge metadata.Jul 8 2016, 5:16 PM

The new header ICallPromotionAnalysis.h seems missing.

tejohnson updated this revision to Diff 63360.Jul 8 2016, 5:21 PM
tejohnson edited edge metadata.

Added missing new header file to patch:
include/llvm/Analysis/IndirectCallPromotionAnalysis.h

tejohnson updated this revision to Diff 63381.Jul 8 2016, 8:11 PM
  • Add another file missed from refactoring
davidxl added inline comments.Jul 8 2016, 8:55 PM
include/llvm/Analysis/IndirectCallPromotionAnalysis.h
61

This analysis does not check legality, why is that?

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
166

Document the parameters including the new one.

287

I think there is a bug in the original code. This check should be moved after legality check below. The update of NumOfPGOICallPromotion should also be done here instead of in tryToPromote.

But this can be fixed in a different patch.

xur added inline comments.Jul 11 2016, 10:50 AM
lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
287

Which check are you referring to?
These early exits seem good to me: for the *Only check, the icall type only depends on the instruction. it has nothing to do with legality.
For the cutoff check, only it reaches, we can safely exit without doing the other checks. why you think this is a bug?

620

is the last argument correct? I think it should be NumCandidates.

xur added inline comments.Jul 11 2016, 11:29 AM
include/llvm/Analysis/IndirectCallPromotionAnalysis.h
61

I guess this can be a super set of the real transformations. this just to add edges.

lib/Analysis/IndirectCallPromotionAnalysis.cpp
70

icp-call-only and -icp-invoke-only are mainly for debug. I think it's better to keep together with the transformation code.

tejohnson added inline comments.Jul 11 2016, 11:36 AM
include/llvm/Analysis/IndirectCallPromotionAnalysis.h
61

Because the legality check is based on the target function, and this is being refactored so that it can be called when the target function is not yet known (when building the summary in the ThinLTO compile step where it is an inter-module indirect call).

tejohnson added inline comments.Jul 11 2016, 11:45 AM
include/llvm/Analysis/IndirectCallPromotionAnalysis.h
61

I guess this can be a super set of the real transformations. this just to add edges.

Right

lib/Analysis/IndirectCallPromotionAnalysis.cpp
70

Why not have them here and prevent adding the ThinLTO edges in the debug case?

lib/Transforms/Instrumentation/IndirectCallPromotion.cpp
166

Ok

620

Yes I think you are right, let me fix that.

davidxl added inline comments.Jul 12 2016, 10:40 AM
include/llvm/Analysis/IndirectCallPromotionAnalysis.h
61

ok.

lib/Analysis/IndirectCallPromotionAnalysis.cpp
70

sounds ok to put them here.

I am testing the fixed patch, should upload it shortly.

lib/Analysis/IndirectCallPromotionAnalysis.cpp
70

Actually, after discussing with Rong, I have moved these back.

tejohnson updated this revision to Diff 63697.Jul 12 2016, 11:07 AM
  • Address review comments.
xur accepted this revision.Jul 12 2016, 11:25 AM
xur edited edge metadata.

lgtm.

This revision is now accepted and ready to land.Jul 12 2016, 11:25 AM
This revision was automatically updated to reflect the committed changes.