pcc added inline comments.
================
Comment at: clang/include/clang/CodeGen/BackendUtil.h:51
+ llvm::Expected<llvm::BitcodeModule>
+ FindThinLTOModule(llvm::MemoryBufferRef MBRef);
}
----------------
mehdi_amini wrote:
> Indentation seems strange?
Yes, it's what clang-format does though.
================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:867
+Expected<BitcodeModule> clang::FindThinLTOModule(MemoryBufferRef MBRef) {
+ Expected<std::vector<BitcodeModule>> BMsOrErr = getBitcodeModuleList(MBRef);
----------------
tejohnson wrote:
> Would it be better to have this in llvm (e.g. in BitcodeReader like
> getBitcodeModuleList), or do you anticipate that we will never need that
> functionality within llvm itself?
Maybe -- at this point I feel that we are shoehorning far too much thinlto
backend functionality into clang and that we should think about making it into
a separate tool. If we ever create such a tool, we can easily move this
function there.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:841
+std::unique_ptr<llvm::Module> CodeGenAction::loadModule(MemoryBufferRef MBRef)
{
+ CompilerInstance &CI = getCompilerInstance();
----------------
mehdi_amini wrote:
> Can you extract this function in a separate patch? I feel the diff could be a
> lot more contained here.
>
Done for this function as well as loadModule in r292970 and r292972.
================
Comment at: clang/lib/CodeGen/CodeGenAction.cpp:848
+ // the correct module out of a multi-module bitcode file.
+ if (!CI.getCodeGenOpts().ThinLTOIndexFile.empty()) {
+ VMContext->enableDebugTypeODRUniquing();
----------------
tejohnson wrote:
> What should happen if we have a multi-module bitcode file but no
> ThinLTOIndexFile? Right now I think that would error (with or without this
> patch), right? Do we instead want to compile the non-ThinLTO module, or
> compile the ThinLTO module without ThinLTO (which is what would happen if you
> passed a single-module bitcode file to clang without -fthinlto-index).
Yes, we'd error out in that case, and I think that's fine. Basically at the
moment we only support passing ThinLTO output to either the linker or the thin
backend, and anything else is unsupported.
================
Comment at: clang/test/CodeGen/thinlto_backend.ll:26
+; RUN: llvm-cat -b -o %t5.o %t1.o %t2merge.o
+; RUN: %clang -target x86_64-unknown-linux-gnu -O2 -o %t6.o -x ir %t5.o -c
-fthinlto-index=%t.empty.thinlto.bc -mllvm -ignore-empty-index-file
+; RUN: llvm-nm %t5.o | FileCheck --check-prefix=CHECK-OBJ-IGNORE-EMPTY %s
----------------
tejohnson wrote:
> Are we testing the right thing here? How do we know we have loaded the
> correct module when passed an empty index file which causes us to drop to
> non-ThinLTO compilation with -ignore-empty-index-file? Maybe that handling is
> later, but it isn't clear from this test - can you instead test using an
> index file that enables ThinLTO compilation?
Done, and split out into a separate test.
https://reviews.llvm.org/D29067
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
