Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Prototype of modules codegen

Authored by dblaikie on Jan 17 2017, 8:08 PM.



First pass at generating weak definitions of inline functions from module files
(& currently skipping definitions of those functions in uses)

I've done some manual testing (haven't delved into the preferred testing
strategy for modules). Seems to work for simplest cases, including transitive

Also doesn't have an actual flag plumbed through, yet. I can submit that ahead
of this in a separate patch (open to ideas, etc). Is there a goal for how
explicit modules should be used via the driver (rather than cc1)? Otherwise
this would be a cc1 option to match.

One quandry: What to do with non-inline definitions in headers? Currently this
prototype moves their emission (while keeping their linkage) to the module
object. This would remove ODR violations and actually make it 'correct' to
define a non-inline function in a header, which may or may not be desirable. It
could be kept in all the users as it is today.

I'm sure I don't know nearly enough about linkage and about how linkage is
handled in Clang & LLVM. So I'm certainly open to ideas about how this
implementation isn't ideal or correct - or the myriad of interesting linkage
cases I should test/consider.

Perhaps we could chat about this in person/over the shoulder if a faster
iteration would be worthwhile.

Diff Detail


Event Timeline

dblaikie created this revision.Jan 17 2017, 8:08 PM

Can you provide a high-level description of what you're trying to accomplish and the usage model?

dblaikie updated this revision to Diff 85484.Jan 23 2017, 4:17 PM

Add bit to the Module record for when modular codegen decls are included in the MODULAR_CODEGEN_DECLS bitcode record

dblaikie updated this revision to Diff 85599.Jan 24 2017, 9:44 AM
  • Move linkage handling into GetGVALinkageForFunction
dblaikie updated this revision to Diff 85649.Jan 24 2017, 4:06 PM
  • Add cc1 flag for modular codegen, -fmodule-codegen
dblaikie updated this revision to Diff 85686.Jan 24 2017, 7:39 PM
  • Add test coverage
dblaikie updated this revision to Diff 86140.Jan 27 2017, 3:36 PM
  • More test coverage (for local static variables)
rsmith edited edge metadata.Jan 27 2017, 4:00 PM
2490 ↗(On Diff #85686)

I think the name of this flag might be out of sync with its meaning; it looks like it means "must this declaration be emitted when performing modular codegen?"

208 ↗(On Diff #85686)

Does this need to be two bits wide? Seems to only store true/false.

435–438 ↗(On Diff #85686)

Maybe module -> modules, to match the other -fmodules-* flags controlling various options.

9020–9023 ↗(On Diff #85686)

I think this return true is unreachable and can be deleted: if we get here with Linkage == GVA_DiscardableODR, then the call to hasExternalDefinitions in GetGVALinkageForFunction must have returned EK_ReplyHazy. (In the case this is checking for, GetGVALinkageForFunction would return GVA_StrongODR, so the check below will return true anyway.)

9029 ↗(On Diff #85686)

Pass Linkage in here instead of recomputing it

33 ↗(On Diff #85686)

You should add support for this function to clang::MultiplexExternalSemaSource too.

2215–2219 ↗(On Diff #85686)

I suspect we'll want to seriously prune back the contents of EagerlyDeserializedDecls for the modular codegen case at some point, but we don't need to do that in this change.

dblaikie updated this revision to Diff 86200.Jan 28 2017, 11:38 PM
  • Address code review feedback
  • Formatting
dblaikie marked 6 inline comments as done.Jan 28 2017, 11:47 PM

Addressed CR feedback

33 ↗(On Diff #85686)

Done - though is there any test coverage I should add? Not sure exactly when/where/how this is used.

Also only made a rough guess at what the right behavior is here. It could be a bit more obvious if I made "hasExternalDefinitions" return Optional<ExtKind> - then when we find an ExternalASTSource that owns the ID (are the IDs unique across the MultiplexExternalSemaSource? I assume they have to be, but thought they'd only be valid within a single Module... guess I'm confused in a few ways - certainly haven't thought about it in great detail) we'd know to stop asking other sources. Probably not worth it unless there's a lot of sources?

2215–2219 ↗(On Diff #85686)

Right - I was wondering if we'd end up with anything in EagerlyDeserializedDecls when modular codegen is fully implemented. (if that's the case - we could have only one list, and a 'bit' to say whether it's modular codegen decls or EagerlyDeserializedDecls, if these records/blobs/whatnot are expensive to have two of rather than one - but I don't think that's the case so probably more readable/self-documenting to use two lists as we are)

rsmith accepted this revision.Jan 29 2017, 10:21 AM
rsmith added inline comments.
33 ↗(On Diff #85686)

You could test this with -chain-include. I don't think there's any other in-tree way to get multiple external sources.

This revision is now accepted and ready to land.Jan 29 2017, 10:21 AM
This revision was automatically updated to reflect the committed changes.
dblaikie marked 2 inline comments as done.