Page MenuHomePhabricator

[NFC] Do not run CGProfilePass when not using integrated assembler
ClosedPublic

Authored by zhizhouy on May 29 2019, 1:53 PM.

Details

Summary

CGProfilePass is run by default in certain new pass manager optimization pipeline. Assemblers other than llvm as (such as gnu as) cannot recognize the .cgprofile entries generated and emitted from this pass, causing build time error.

This patch adds new options in clang CodeGenOpts and PassBuilder options so that we can turn cgprofile off when not using integrated assembler.

Diff Detail

Event Timeline

zhizhouy created this revision.May 29 2019, 1:53 PM

I find it a bit odd to have clang care about this, but I suppose it is the driver that knows which features tools support. I also have an issue with the name as CGProfilePass doesn't actually do the sorting. I would just keep the option as -enable-call-graph-profile-emission or something like that.

IIUC, user will always call clang to pass integrated as flag. Actually I also used to consider checking UseIntegratedAS-ish in PassBuilder,
but looking for a codegen options in PassBuilder seems going to break the integrity. Solutions for this is appreciated.

I also have an issue with the name as CGProfilePass doesn't actually do the sorting

Sorry I mis-used the "sort" here, changing it to "enable-npm-call-graph-profile" to match the other newpm options in PassBuilder.

zhizhouy updated this revision to Diff 202057.May 29 2019, 2:34 PM

Renaming the opt.

zhizhouy edited the summary of this revision. (Show Details)May 29 2019, 2:39 PM
manojgupta added inline comments.May 29 2019, 2:41 PM
clang/lib/Driver/ToolChains/Clang.cpp
4000 ↗(On Diff #202057)

llvm MC assembler? llvm-as is for converting text ll to bitcode files.

zhizhouy updated this revision to Diff 202065.May 29 2019, 2:46 PM
zhizhouy marked 2 inline comments as done.
zhizhouy edited the summary of this revision. (Show Details)

Fixed comment.

clang/lib/Driver/ToolChains/Clang.cpp
4000 ↗(On Diff #202057)

Thanks for correction.

IIUC, user will always call clang to pass integrated as flag. Actually I also used to consider checking UseIntegratedAS-ish in PassBuilder,
but looking for a codegen options in PassBuilder seems going to break the integrity. Solutions for this is appreciated.

There are frontends and users of llvm other than clang.

Also all of the other options there are useful for experimenting with new passes, none of them are required for correctness, and none of them are ever set by clang.

Adding Chandler to see if he has a good solution.

Friendly ping :)

chandlerc added inline comments.Jul 11 2019, 5:40 PM
clang/lib/Driver/ToolChains/Clang.cpp
3999–4003 ↗(On Diff #202065)

the call garph profile stuff needs to be a Clang CodeGenOpt IMO. Then it can be used to set a bool in the pass builder's option struct (see my other comment).

We can then set the CodeGenOpt based on whether we are using the integrated assembler.

We could also have a separate flag for setting the codegen opt for getting this profile information, and make the driver diagnose an incompatible combination of no-integrated-as and that flag. But we'll still end up threading that flag through to Clang's codegen layer, and then into the pass manager.

llvm/lib/Passes/PassBuilder.cpp
1068–1069

We shouldn't just use the cl::opt here. We need a flag in the pass builder's options struct. We can default it from the cl::opt.

You should add a test that uses the cl::opt to flip the default back and forth and get the different behavior in LLVM directly against the opt tool. Then the LLVM part of this can land.

zhizhouy updated this revision to Diff 252129.Mar 23 2020, 1:38 PM
zhizhouy marked 2 inline comments as done.
zhizhouy edited the summary of this revision. (Show Details)
zhizhouy added a reviewer: manojgupta.
zhizhouy removed a subscriber: manojgupta.
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2020, 1:38 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Thanks,

Noticed a few typos. Rest lgtm but deferring to other reviewers for now for approval.

clang/lib/CodeGen/BackendUtil.cpp
1111

Typo: PTO.CallGrpahProfile -> PTO.CallGraphProfile

1522

Same typo.

llvm/include/llvm/Passes/PassBuilder.h
111

CallGrpahProfile -> CallGraphProfile

xur added a comment.Mar 23 2020, 2:21 PM

look good to me with the fix of comments.

llvm/include/llvm/Passes/PassBuilder.h
110

The default value is specified by -enable-npm-call-graph-profile, rather than true, right?

zhizhouy updated this revision to Diff 252143.Mar 23 2020, 2:38 PM
zhizhouy marked 4 inline comments as done.
zhizhouy retitled this revision from [NFC] Do not run CGProfilePass when -fno-integrated-as is on to [NFC] Do not run CGProfilePass when not using integrated assembler.

Thanks for the comments.

void edited subscribers, added: manojgupta; removed: cfe-commits.Mar 31 2020, 2:16 AM

Friendly ping. :-)

manojgupta accepted this revision.Mar 31 2020, 9:23 AM
This revision is now accepted and ready to land.Mar 31 2020, 9:23 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added a subscriber: MaskRay.Jul 7 2020, 3:52 PM

I do not see a strong argument for -enable-npm-call-graph-profile. We just need Opts.CallGraphProfile = !Opts.DisableIntegratedAS
The option does not appear to be very useful to me.