This is an archive of the discontinued LLVM Phabricator instance.

Second for inlining report
Needs RevisionPublic

Authored by rcox2 on Apr 21 2016, 5:44 PM.

Details

Summary

Second for inlining report

This is the second in a series of three patches for the inlining report. This annotates the InlineCost.[h,cpp] to include reasons for inlining. Not all of the reasons are included here, as some are handled in the third patch which has more changes to Inliner.cpp. The previous patch was D19397.

Diff Detail

Event Timeline

rcox2 updated this revision to Diff 54601.Apr 21 2016, 5:44 PM
rcox2 retitled this revision from to Second for inlining report .
rcox2 updated this object.
rcox2 added reviewers: reames, hfinkel, apilipenko.
rcox2 added subscribers: kbsmith1, llvm-commits.
reames added inline comments.May 5 2016, 6:00 PM
include/llvm/Analysis/InlineCost.h
48

Fundamentally, I don't think this is the right abstraction. You have a set of reasons that you're trying to squeeze down into a single value. We're probably better off exposing all the contributing reasons to the frontend with a ranking to indicate how important they are.

The feedback on the earlier patch seems to be leading in the same direction. I'm not going to review this patch further until that part has been worked out.

55

enum X {

58

style: please don't be concise over understandable.

149

Having versions without reasons are suspicious.

lib/Analysis/InlineCost.cpp
221

You probably want a reference here.

960

Probably better to populate a local variable and discard it. Optional params via pointers tend to be very error prone.

1394

The long chain of modifications hints that maybe we should just be returning the reason we didn't inline and converting that into a boolean once.

reames requested changes to this revision.May 5 2016, 6:00 PM
reames edited edge metadata.
This revision now requires changes to proceed.May 5 2016, 6:00 PM