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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
