This is an archive of the discontinued LLVM Phabricator instance.

Accept 'clang++ -c a.pch -o a.o' to create PCH's object file
ClosedPublic

Authored by llunak on Jul 13 2020, 1:35 PM.

Details

Summary

This way should be the same like with a.pcm for modules.
An alternative way is 'clang++ -c empty.cpp -include-pch a.pch -o a.o -Xclang -building-pch-with-obj', which is what clang-cl's /Yc does internally.

Diff Detail

Event Timeline

llunak created this revision.Jul 13 2020, 1:35 PM

Could you add a test case?

(/maybe/ two test cases - one for the driver, one for the frontend)

This is tested in D83623 (I'd prefer not to split the changes to the test to reflect both the state before and after D83623).

This is tested in D83623 (I'd prefer not to split the changes to the test to reflect both the state before and after D83623).

Code should be tested in the commit that adds the code & commit should be separate into individual pieces of functionality (so potentially splitting even this patch in two - one for the frontend change & test for it (which I guess is the test change from D83623) and another for the driver change (which isn't currently tested in either review, I think)).

I don't think this would require much churn in the test cases - seems like it's additive? The other review would introduce -fpch-codegen/-fpch-debuginfo which would be purely driver options & get an added driver test there for them & the documentation along with them.

llunak updated this revision to Diff 277670.Jul 13 2020, 10:34 PM

Adjusted test/PCH/codegen.cpp to use/test the change (not much point in testing it without codegen, as in that case the shared object will be empty).

dblaikie added inline comments.Jul 14 2020, 4:20 PM
clang/test/PCH/codegen.cpp
39–50

where's the change to dso_local/not-dso_local come from? Is that expected/good/correct?

llunak marked an inline comment as done.Jul 14 2020, 10:00 PM
llunak added inline comments.
clang/test/PCH/codegen.cpp
39–50

The driver adds "-mrelocation-model static" (by default, in the absence of -fPIC etc.), while the frontend defaults to "-mrelocation pic". I don't know about correct, but here it's harmless/unrelated.

dblaikie added inline comments.Jul 15 2020, 11:06 AM
clang/test/PCH/codegen.cpp
39–50

Ah, sorry, didn't see there was a driver test here.

The driver shouldn't be tested here - it should be tested in test/Driver and should only test the driver functionality (using -### to test that the commands the driver plans to execute have the desired arguments) - one of the reasons the driver functionality should probably be in a separate commit - it's separate/isolated functionality.

llunak marked an inline comment as done.Jul 15 2020, 11:19 AM
llunak added inline comments.
clang/test/PCH/codegen.cpp
39–50

I don't understand what you mean. Are you asking for a test that simply checks that 'clang++ -c a.pch' results in 'clang -cc1 a.pch' (or whatever is is)?

dblaikie added inline comments.Jul 15 2020, 11:37 AM
clang/test/PCH/codegen.cpp
39–50

Yep! that's how the driver's tested. for example: https://github.com/llvm/llvm-project/commit/9914c3a2bae84bb92e5269165c4d098140549b46

Though, honestly, if you're not planning to use this functionality (only using /Yc instead) I'd suggest maybe it's not time to implement a driver flag for this, if it's going to go unused (or without a clear user/use case/validation that this is needed/ongoing exercise of the functionality, etc). & maybe this patch (or patches, if it gets split up) isn't suitable right now. (basically the reason I haven't added -fmodules-codegen driver flags either, since it hasn't been shipped/picked up directly by anyone (though the underlying functionality's exercised by C++20 standard modules which requires codegen - and somewhat by /Yc, moreso if we can merge the functionality a bit more as proposed))

llunak added inline comments.Jul 15 2020, 12:15 PM
clang/test/PCH/codegen.cpp
39–50

I'm not going to use /Yc. I'm going to use -Xclang -building-pch-with-obj. That's why I initially went with -fbuilding-pch-with-obj, until you said that the proper approach is this patch.

And what you're saying looks like Catch-22: No driver flag until somebody uses it, but who's going to use if there's no driver flag? Although if the situation ends up being that Clang11 has the functionality but only "secret" flags to enable it, then we'll get to find out.

dblaikie added inline comments.Jul 15 2020, 12:21 PM
clang/test/PCH/codegen.cpp
39–50

Not entirely - just that we don't often add speculative features to the compiler - we add stuff that someone (usually the person contributing it) has a use for. So they add the feature and start using it, avoiding the Catch 22. (if it were a retail product, where the people developing it are distinct from the people using it - yeah, in that case there's more speculative implementation and/or user studies, etc)

But, yes, having -Xclang flags that make it clear this isn't a shipping feature (though we've done that even more agressively in some cases - see the rather ominously named "-enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang") still lets folks experiment with it (as I've done with modular codegen) & see how it plays out, what the most desirable interface to the feature would be, etc. Which is already the state - users could use -Xclang -fmodules-codegen.

Oh, I guess by secret you mean the flags you're proposing, but instead not documenting them so as not to overly publicize them?

By "secret" flags I mean prefixing them with -Xclang. But I guess that's how this situation will turn out. If this one and D83623 don't make it into Clang11, then people will need to use the -Xclang ones, and then these new ones don't make much sense, because who'll support one set of flags now and implement an optional new set of flags half a year later merely to have a slightly nicer spelling of the flags.

As for my use and not documenting the flags, I've been using this for the last 9 months and with the review process dragging so much I would have given up a long time ago if I cared only about myself. Saving 20% of build time sounds like something that could be easily used by many others. I'm rather sure I want to publicize this.

Anyway, so what now? Should I scratch this and D83623, or should I change D83623 to only document the flags as the -Xclang variants, or what to do?

dblaikie accepted this revision.Jul 17 2020, 1:51 PM

By "secret" flags I mean prefixing them with -Xclang. But I guess that's how this situation will turn out. If this one and D83623 don't make it into Clang11, then people will need to use the -Xclang ones, and then these new ones don't make much sense, because who'll support one set of flags now and implement an optional new set of flags half a year later merely to have a slightly nicer spelling of the flags.

It's certainly happened before - internal flags for limited prototyping/testing/experimentation while finalizing the functionality, etc.

But sure, let's go with public driver flags for this.

Please split this patch (or at least move the driver testing out of test/PCH into test/Driver) for the driver portion separately from the frontend support.

So then it'd be 3 patches:

  • frontend/cc1 support for compiling pch files directly (rather than only indirectly via -building-pch-with-obj)
  • driver support for compiling .pch files the same as module files
  • driver flags -fpch-codegen/-fpch-debuginfo and documentation for that use case
This revision is now accepted and ready to land.Jul 17 2020, 1:51 PM
llunak updated this revision to Diff 278997.Jul 18 2020, 6:58 AM

Moved driver test under test/Driver/.

dblaikie added inline comments.Jul 20 2020, 2:25 PM
clang/test/Driver/pch-codegen.cpp
17–18

FWIW, this is generally not/should not be tested.

Please only test the Driver (clang -###) in test/Driver and only test the Frontend (clang _cc1) in the other directories.

29–41

This is already covnered in the PCH/codegen.cpp test, right? (except for the driver interaction - and that's already tested above with the driver tests)

llunak marked 2 inline comments as done.Jul 21 2020, 12:42 AM
llunak added inline comments.
clang/test/Driver/pch-codegen.cpp
17–18

The test below errors out if the PCH does not exist, already in the driver code. I can change the comment to "Actually create the PCH, needed by the tests below." if that helps.

29–41

No. The tests above test creating the PCH. This part tests creating the PCH's object file. When testing PCH codegen, it makes sense to test both of these.

dblaikie added inline comments.Jul 21 2020, 12:30 PM
clang/test/Driver/pch-codegen.cpp
17–18

Ah, it doesn't read the file though, right? so could you change this to touch or otherwise create an empty file to test the driver with? (*checks* Yeah, there's a bunch of other tests in clang/test/Driver that use touch to create input files for Driver testing)

29–41

Oh, right, sorry, missed the ### there!

llunak updated this revision to Diff 279624.Jul 21 2020, 1:30 PM

Updated driver test to just create a dummy file for PCH.

dblaikie accepted this revision.Jul 21 2020, 1:32 PM

Looks good - thanks!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 22 2020, 1:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript