[PATCH] D68049: Propeller: Clang options for basic block sections
Sriraman Tallam via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Feb 11 10:15:01 PST 2020
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
More information about the cfe-commits
mailing list