This is an archive of the discontinued LLVM Phabricator instance.

Pass AssumptionCacheTracker from SampleProfileLoader to Inliner
ClosedPublic

Authored by danielcdh on Jun 9 2016, 2:47 PM.

Details

Summary

Inliner needs ACT when calling InlineFunction. Instead of nullptr, we need to pass it in from SampleProfileLoader

Diff Detail

Event Timeline

danielcdh updated this revision to Diff 60243.Jun 9 2016, 2:47 PM
danielcdh retitled this revision from to Pass AssumptionCacheTracker from SampleProfileLoader to Inliner.
danielcdh updated this object.
danielcdh added a reviewer: davidxl.
danielcdh added a subscriber: llvm-commits.
davidxl added inline comments.Jun 9 2016, 3:02 PM
lib/Transforms/IPO/SampleProfile.cpp
1282

add a FIXME comment for new pm here.

danielcdh updated this revision to Diff 60251.Jun 9 2016, 3:08 PM

add FIXME

davidxl edited edge metadata.Jun 9 2016, 3:09 PM

Is it possible to extract a test demonstrating the problem?

danielcdh updated this revision to Diff 60288.Jun 9 2016, 5:30 PM
danielcdh edited edge metadata.

Add unittest

ping...

Thanks,
Dehao

Is the testing just testing opt not crashing?

Please cleanup the test case: shorten the names, and minimizes the number of debug info needed.

test/Transforms/SampleProfile/inline-act.ll
4

empt --> empty

danielcdh updated this revision to Diff 60781.Jun 14 2016, 5:14 PM

trim debug info

vsk added a subscriber: vsk.Jun 15 2016, 9:52 AM

It looks like the key parts of this test case are: a load/store to a global, an inline candidate with a switch in it, and debug information.

Are there any elements there that are unnecessary, or that could be trimmed down further? I'm asking because I'm not very familiar with the SampleProf code, and am having a hard time working out exactly how inline-act.ll works. It would help to have SampleProfileLoader dump some debug output that can be FileChecked.

This test is just testing if SampleLoader will crash under certain conditions. You are right, the crash will only happen when 3 of these conditions satisfy. It's more like a robustness test, so FileCheck may not be an option here.

eraman added a subscriber: eraman.Jun 16 2016, 3:45 PM

This test is just testing if SampleLoader will crash under certain conditions. You are right, the crash will only happen when 3 of these conditions satisfy. It's more like a robustness test, so FileCheck may not be an option here.

IT looks like this use of IFI is at the InlineFunction method where a phi node is inserted when the callee has multiple exits and the result of the call is used. I think the test case can be simplified without needing to have any debug information. Alternatively, there are other places where IFI is used without any guards - for example when there is a pointer parameter with a specified alignment.

davidxl accepted this revision.Jun 20 2016, 9:31 AM
davidxl edited edge metadata.

lgtm

Easwaran, can you help investigate the inliner to see if there is anything general to be done there -- either tighten the interface if ACT is always required or something else.

This revision is now accepted and ready to land.Jun 20 2016, 9:31 AM
danielcdh closed this revision.Jun 20 2016, 2:00 PM