[PATCH] D34245: [PowerPC] Refine the checking for emiting TOC restore nops and Tail-Call eligibility.

Hal Finkel via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 08:09:09 PDT 2017


hfinkel added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:4264
+
+  // Check if Callee resides in the same section, because for Small codemodel,
+  // we could end up with multiple TOCs in the same module.
----------------
codemodel should either be "code model" or "CodeModel". My preference is to just write this in English, "small code model", with no extra capitalization.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:4265
+  // Check if Callee resides in the same section, because for Small codemodel,
+  // we could end up with multiple TOCs in the same module.
+  // ref: https://bugzilla.mozilla.org/show_bug.cgi?id=973977
----------------
This does not explain why this cannot happen for the other code models. The ABI is a bit ambiguous here, saying "The medium code model is expected to provide a sufficiently large TOC to provide all data addressing needs of a module with a single TOC." Expected, however, does not mean "must". We need to supplement here with some additional explanation of what existing linkers actually do (e.g. that neither ld.bfd nor ld.gold will create multiple TOCs for anything other than the small code model, regardless of what might be theoretically allowed).


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:4266
+  // we could end up with multiple TOCs in the same module.
+  // ref: https://bugzilla.mozilla.org/show_bug.cgi?id=973977
+  return !resideInSameSection(Caller, Callee, TM);
----------------
I don't see exactly how this reference is relevant. There is some explanation of why you can't tail call to functions in other modules, but if that's not explained in the comments in this file, we should do that directly.


Repository:
  rL LLVM

https://reviews.llvm.org/D34245





More information about the llvm-commits mailing list