[PATCH] D68049: Propeller: Clang options for basic block sections

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram marked 13 inline comments as done.
tmsriram added inline comments.


================
Comment at: clang/include/clang/Basic/CodeGenOptions.def:341
+CODEGENOPT(RelocateWithSymbols, 1, 0)
+
 /// Whether we should use the undefined behaviour optimization for control flow
----------------
mehdi_amini wrote:
> Can you add a doc here? (possibly referring to somewhere else if it is extensively documented elsewhere?)
I am unable to find a doc for this.  I added comments on this based on what "shouldRelocateWithSymbol" in ELFObjectWriter does and why it prefers relocating with sections rather than symbols.


================
Comment at: clang/include/clang/Driver/Options.td:1873
+def fbasicblock_sections_EQ : Joined<["-"], "fbasicblock-sections=">, Group<f_Group>, Flags<[CC1Option, CC1AsOption]>,
+  HelpText<"Place each function's basic blocks in unique sections (ELF Only) : all | labels | none | <filename>">;
 def fdata_sections : Flag <["-"], "fdata-sections">, Group<f_Group>,
----------------
mehdi_amini wrote:
> Is the "labels" options dependent/related to the previous -fpropeller-label one?
Yes, -fpropeller-label is the umbrella propeller option that turns on -fbasicblock-sections=labels.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:348
+        Twine NewName = Section + Name;
+        Fn->setSection(NewName.str());
+      }
----------------
mehdi_amini wrote:
> Is this related to these new option? It this change the behavior of -ffunction-sections?
Yes, this is related to function sections getting it right which is important for Propeller.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1032
     }
-
+  }
   return Out.str();
----------------
mehdi_amini wrote:
> I agree with the improvement, but as nit this isn't related to the current patch or even in a function you're otherwise touching. (it creates an extra hunk in the review)
Removed it and will add it independently.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:1103
+      dyn_cast<FunctionDecl>(GD.getDecl()) &&
+      this->getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {
+    std::string UniqueSuffix = getUniqueModuleId(&getModule(), true);
----------------
MaskRay wrote:
> ```
>   if (getCodeGenOpts().UniqueInternalFuncNames &&
>       isa<FunctionDecl>(GD.getDecl()) &&
>       getFunctionLinkage(GD) == llvm::GlobalValue::InternalLinkage) {
> ```
> 
> How does it interop with `MangledName = getCUDARuntime().getDeviceStubName(MangledName);` below?
The .stub will be suffixed and it will demangle correctly.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:631
+    if (A->getOption().matches(options::OPT_fpropeller_optimize_EQ)) {
+      if (!Args.getLastArgValue(options::OPT_fuse_ld_EQ).equals_lower("lld"))
+        D.Diag(clang::diag::err_drv_unsupported_opt)
----------------
MaskRay wrote:
> This check is overly constrained. Some systems default to use lld (e.g. installed at /usr/bin/ld). I suggest removing this check.
I see what you mean, can we follow this up in some manner with a check to see if the underlying linker supports Propeller?  


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:640
+      CmdArgs.push_back("--optimize-bb-jumps");
+      CmdArgs.push_back("--no-call-graph-profile-sort");
+      CmdArgs.push_back("-z");
----------------
MaskRay wrote:
> Why --no-call-graph-profile-sort?
Call graph profile sort conflicts with the reordering we do and cannot be on at the same time.  We have plans to merge the two, so until then.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:642
+      CmdArgs.push_back("-z");
+      CmdArgs.push_back("nokeep-text-section-prefix");
+      CmdArgs.push_back("--no-warn-symbol-ordering");
----------------
MaskRay wrote:
> This will silently ignore user specified `-z keep-text-section-prefix`.
> 
> With `-z nokeep-text-section-prefix`, an input section `.text.hot.foo` will go to the output section `.text`, instead of `.text.hot`. Why do you need the option?
We are planning to restore keep-text-section-prefix in some manner with Propeller.  Since propeller shuffles sections what is hot is not clearly defined by a prefix so this option won't make sense with Propeller.  We will use a heuristic to compute hotness and then regenerate the section markers in the final binary.  


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

https://reviews.llvm.org/D68049