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

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
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)
----------------
tmsriram wrote:
> 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?  
Some systems install lld at /usr/bin/ld. This will work even if -fuse-ld=lld is not specified. lld can also be used with -fuse-ld=/path/to/ld.lld . I think the best is just not to have the check.


================
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");
----------------
tmsriram wrote:
> 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.  
OK, thanks for the clarification. The two disabled features deserve comments, even if they are TODO.


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

https://reviews.llvm.org/D68049