tmsriram added a comment. 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.. 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`. ================ Comment at: clang/lib/CodeGen/BackendUtil.cpp:444 + while ((std::getline(fin, line)).good()) { + StringRef S(line); + // Lines beginning with @, # are not useful here. ---------------- grimar wrote: > Something is wrong with the namings (I am not an expert in lib/CodeGen), but > you are mixing lower vs upper case styles: "fin", "line", "S", "R". Seems the > code around prefers upper case. I am moving this function out of clang into llvm as this needs to be shared by llc, llvm and lld. I will address all your comments for this function in the llvm change. 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
