[PATCH] D68062: Propeller lld framework for basicblock sections

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/Propeller.h:195
+
+  bool openPropf() {
+    this->PropfStream.open(this->PropfName);
----------------
shenhan wrote:
> ruiu wrote:
> > This function is called only once, please inline.
> Added "inline" keyword. Or do you mean to inline and remove the function body?
I meant the latter.


================
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);
+  };
----------------
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).


================
Comment at: lld/include/lld/Common/PropellerCommon.h:1
+#ifndef LLD_ELF_PROPELLER_COMMON_H
+#define LLD_ELF_PROPELLER_COMMON_H
----------------
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.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D68062