[PATCH] D68062: Propeller lld framework for basicblock sections

[Han Shen via Phabricator via llvm-commits]

shenhan added inline comments.


================
Comment at: lld/ELF/Propeller.h:257
+
+  void reportProfError(const StringRef msg) const;
+
----------------
ruiu wrote:
> You should avoid using `Prop` or `Propeller` as a part of an identifier for Propeller, because essentially everything you are writing is for Propeller. So it's not very explanatory. For example, I'd name this reportParseError.
> 
> Please remove `const` from `const StringRef`.
Yes. Also s/checkPropellerTarget/checkTarget/g & s/checkPropellerLegacy/checkLegacy/g.


================
Comment at: lld/ELF/Propeller.h:280-284
+  // ELFViewDeleter, which has its implementation in .cpp, saves us from having
+  // to have full ELFView definition visibile here.
+  struct ELFViewDeleter {
+    void operator()(ELFView *v);
+  };
----------------
ruiu wrote:
> shenhan wrote:
> > ruiu wrote:
> > > I'm puzzled by this -- do you really need this?
> > Yes, unique_ptr<Type> requires Type's dtor to be visible at this place. However, Propeller.h is the top level hdr file within propeller framework, thus it could not include any other propeller hdr file (only the other way around), so we don't have ELFView (now ObjectView after renaming)'s full type defintion here.
> > 
> > To overcome this, we provide a customer deleter functor - ELFViewDeleter, and define it in the ELFView's cpp file.
> But I think you can fix that error by moving the definition of this class's ctor to a .cpp file (i.e. don't inline).
Yes, alternatively, we can move Propeller's ctor *AND* dtor to Propeller.cpp, that also implies putting an empty "dtor" definition in the .cpp. Because compiler-synthesized dtors are inlined. What shall we do, keep the code as is, or move ctor and put an empty dtor in the .cpp? I am ok w/ either.


================
Comment at: lld/include/lld/Common/PropellerCommon.h:1
+#ifndef LLD_ELF_PROPELLER_COMMON_H
+#define LLD_ELF_PROPELLER_COMMON_H
----------------
ruiu wrote:
> shenhan wrote:
> > ruiu wrote:
> > > This header is included only once by another header, so please merge them together.
> > Yup, let me explain. We have the creaet_llvm_prof tool which is part of google/autofdo repository (https://github.com/shenhanc78/autofdo/blob/plo-dev/llvm_propeller_profile_writer.h#L14).  create_llvm_prof tool and propeller need to have exactly same symbolentry definition to cooperate. To make a copy of the class definition in google/autofdo repository is not a good idea. So we create this common inclusion hdr file.
> > 
> > 
> This is an lld's private header directory. If you have a header that is shared by multiple LLVM subprojects, consider moving it to LLVM.
I found PropellerCommon.h exported into the llvm installation directory here:
    llvm-install/include/lld/Common
along with llvm-install/include/llvm  and llvm-install/include/clang. 

And I think it's probably inappropriate to place PropellerCommon.h anywhere under llvm-install/include/clang or llvm-install/include/llvm.


Repository:
  rLLD LLVM Linker

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68062/new/

https://reviews.llvm.org/D68062