This is an archive of the discontinued LLVM Phabricator instance.

Cleanup coro-inline.ll
ClosedPublic

Authored by lxfind on Dec 15 2020, 2:45 PM.

Details

Summary

Following up with the comments in D92706.

  • Use -passes instead of -enable-new-pm
  • CoroEarly should happen before AlwaysInliner, adjust it.
  • Remove some unnecessary barriers (still kept one)
  • Cleanup unnecessary debug info

Diff Detail

Unit TestsFailed

Event Timeline

lxfind created this revision.Dec 15 2020, 2:45 PM
lxfind requested review of this revision.Dec 15 2020, 2:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 2:45 PM
dongAxis1944 added inline comments.Dec 15 2020, 6:55 PM
llvm/test/Transforms/Coroutines/coro-inline.ll
2

always-inline should called before coro-early i think ???

aeubanks added inline comments.Dec 15 2020, 11:03 PM
llvm/test/Transforms/Coroutines/coro-inline.ll
1–4

is this one still needed?

3–4

FYI I tried reverting the original change and these still passed

33

Sorry, it does look like the debug info is required for sample-profile to properly work. Although as mentioned above, not sure the test is actually properly testing this.

45

seems like most of these attributes aren't relevant

lxfind added inline comments.Dec 16 2020, 5:34 PM
llvm/test/Transforms/Coroutines/coro-inline.ll
2

This is where CoroEarly is added to the pipeline:
https://github.com/llvm/llvm-project/blob/7ea3932ab1def0f5e86ac745bef0d3de09e8845f/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp#L299

This is where the inliner is added to the pipeline: https://github.com/llvm/llvm-project/blob/7ea3932ab1def0f5e86ac745bef0d3de09e8845f/llvm/lib/Transforms/IPO/PassManagerBuilder.cpp#L539

I tried to run Clang on a coroutine cpp file -O0, both let it print out the pass sequence and also set a few breakpoints. I saw that the CoroEarly pass is always executed before AlwaysInliner in the pipeline.

lxfind updated this revision to Diff 312350.Dec 16 2020, 5:44 PM

Update test. A few things: 1) we don't need and shouldn't add --coro-early. Both functions are already marked with coroutine.presplit=1, ready for CoroSplit. coro-early will actually mess it up. 2) It turns out we need the meta data and dbg info since SampleProfileLoader depends on it. 3) We don't need all the attributes. Removed the unused ones.

This revision is now accepted and ready to land.Dec 17 2020, 5:38 AM
This revision was automatically updated to reflect the committed changes.