steven_wu added a comment.

Hi Richard

Thanks for looking at the patch! Replies are inlined with the feedback.

Steven


================
Comment at: include/clang/Frontend/CodeGenOptions.def:57
@@ -56,1 +56,3 @@
+CODEGENOPT(EmbedBitcode      , 1, 0) ///< Embed LLVM IR bitcode as data.
+CODEGENOPT(EmbedMarkerOnly   , 1, 0) ///< Only create bitcode section as marker
 CODEGENOPT(EmitDeclMetadata  , 1, 0) ///< Emit special metadata indicating what
----------------
rsmith wrote:
> What is this "marker only" mode for?
marker only mode is used to speed up the compilation while still produce enough 
information in the final output to check if all the files containing a bitcode 
section.
-fembed-bitcode option will split the compilation into two stages and it adds 
measurable costs to compile time due to the extra process launch, the 
serialization and the verifier. -fembed-bitcode-marker is just a way to avoid 
that costs but still mark the section for the sanity check later (done by 
linker in our case).

================
Comment at: lib/CodeGen/BackendUtil.cpp:792
@@ +791,3 @@
+
+  // Embed command-line options.
+  ArrayRef<uint8_t> CmdData((uint8_t*)CGOpts.CmdArgs.data(),
----------------
rsmith wrote:
> As mentioned in the discussion on cfe-dev, embedding the command line and 
> embedding bitcode appear to be completely orthogonal and separate concerns. 
> Can you explain why it makes sense to control both with the same flag?
I thought the original discussion is that if the command line is needed for the 
bitcode embedding. I am happy to add option to toggle command line embedding. 
Here is the proposal:
-fembed-bitcode is implying -fembed-bitcode=all that embeds both bitcode and 
command line.
-fembed-bitcode=bitcode only embeds bitcode and no command line.
command line itself is useless so I will not provide an option to do that.

================
Comment at: lib/CodeGen/BackendUtil.cpp:793-794
@@ +792,4 @@
+  // Embed command-line options.
+  ArrayRef<uint8_t> CmdData((uint8_t*)CGOpts.CmdArgs.data(),
+                            CGOpts.CmdArgs.size());
+  llvm::Constant *CmdConstant =
----------------
rsmith wrote:
> This doesn't seem like you're saving enough information to be able to replay 
> the build (you're missing the current working directory, for instance). Can 
> you clarify exactly what you want to be able to do with the embedded 
> command-line options?
From the original discussion, I mentioned that all paths are useless for the 
purpose of reproducing compilation from bitcode input. Bitcode is very 
self-contain that it has all the debug info and other source information 
included. Switching current working directory will not affect the object file 
generated from the bitcode. Please let me know if I missed something.
On the other hand, we have some backend options that will affect the 
compilation result. There are few options that are not properly encoded in the 
bitcode (see D17394) that should be passed from command line to reproduce the 
exact same compilation. The end goal is for command line section is that for 
-fembed-bitcode, there are two stages:
1. source code -> bitcode
2. bitcode -> object file
Command line section should store all the arguments in stage 2 so it can be 
replayed if needed.
Without D17394, too much information is embedded in the command line section 
(warning flags, include paths, etc., all embedded but ignored by clang if input 
is bitcode). D17394 is trying to trim down the options used in stage 2.


http://reviews.llvm.org/D17392



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to