tmsriram added a comment. In D68049#1870094 <https://reviews.llvm.org/D68049#1870094>, @tmsriram wrote:
> In D68049#1868623 <https://reviews.llvm.org/D68049#1868623>, @MaskRay wrote: > > > > In D68049#1865967 <https://reviews.llvm.org/D68049#1865967>, @MaskRay > > > wrote: > > > If you don't mind, I can push a Diff to this Differential which will > > > address these review comments. > > > > I can't because I can't figure out the patch relationship... > > > > First, this patch does not build on its own. I try applying D68063 > > <https://reviews.llvm.org/D68063> first, then this patch. It still does not > > compile.. > Darn, the include of "llvm/ProfileData/BBSectionsProf.h" is missing in BackendUtil.cpp. Made this mistake when splitting the patch. I will fix this shortly. > Weird, getBBSectionsList is defined by D68063 > <https://reviews.llvm.org/D68063>. Let me try doing that again. I will also > address the rest of your comments. > >> >> >> clang/lib/CodeGen/BackendUtil.cpp:484:11: error: no member named >> 'propeller' in namespace 'llvm' >> llvm::propeller::getBBSectionsList(CodeGenOpts.BBSections, >> >> >> Chatted with shenhan and xur offline. I tend to agree that >> -fbasicblock-sections=label can improve profile accuracy. It'd be nice to >> make that feature separate, >> even if there is still a debate on whether the rest of Propeller is done in >> a maintainable way. >> >> I think the patch series should probably be structured this way: >> >> 1. LLVM CodeGen: enables basic block sections. >> 2. clang Driver/Frontend/CodeGen: pass basic block sections options to LLVM. >> 3. LLVM CodeGen: which enables the rest of Propeller options. >> 4. lld: a file similar to lld/ELF/LTO.cpp . It should be a thin wrapper of >> Propeller features. It should not do hacky diassembly tasks. >> 5. clang Driver/Frontend/CodeGen: passes compiler/linker options to 3 and 4 >> >> Making 1 and 2 separate can help move forward the patch series. 1 and 2 >> should not reference `llvm::propeller`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68049/new/ https://reviews.llvm.org/D68049 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
