[PATCH] D153907: [AIX] [TOC] Add -mtocdata/-mno-tocdata options on AIX

[Amy Kwan via Phabricator via llvm-commits]

amyk added a comment.

Here are some group review comments from today's session.

Also, can we summarize the user manual in the patch description?



================
Comment at: clang/docs/UsersManual.rst:3159
 
+Mark TOC Data / Not TOC Data (AIX-specific)
+-------------------------------------------
----------------



================
Comment at: clang/docs/UsersManual.rst:3162
+
+When -mtocdata is specified, all external linkage variables and variables with
+static storage duration, including static data members of classes and
----------------
Add a description to what is done with the toc-data attribute.


================
Comment at: clang/docs/UsersManual.rst:3162
+
+When -mtocdata is specified, all external linkage variables and variables with
+static storage duration, including static data members of classes and
----------------
amyk wrote:
> Add a description to what is done with the toc-data attribute.
We should describe here:
- Add a separate paragraph to explain interface of EQ options only adding exceptions to how the global value rule for the attributes are applied to variables.
- Describe the global option descriptions first in the user manual before the EQ options


================
Comment at: clang/docs/UsersManual.rst:3170
+
+.. option:: -mtocdata=<list>
+
----------------
Add double quotes for options.


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:558
+def warn_drv_unsupported_tocdata: Warning<
+  "ignoring '-mtocdata' with -mcmodel=medium, -mcmodel=large">,
+  InGroup<OptionIgnored>;
----------------
Just say that it's only supported for small code model.


================
Comment at: clang/include/clang/Driver/Options.td:2958
+def mtocdata : Flag<["-"], "mtocdata">, Flags<[CC1Option,TargetSpecific]>,
+  HelpText<"All variables can be treated as toc-data">,
+  MarshallingInfoFlag<CodeGenOpts<"AllTocData">>;
----------------



================
Comment at: clang/include/clang/Driver/Options.td:2965
+def mno_tocdata : Flag<["-"], "mno-tocdata">, Flags<[CC1Option,TargetSpecific]>,
+  HelpText<"Only explicitly specified variables will be treated as toc-data">;
 defm preserve_as_comments : BoolFOption<"preserve-as-comments",
----------------
Can we move this under the option on 2957 (put positive and negative options together) and ensure the HelpTexts are updated accordingly?


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:421
     Action::OffloadKind DeviceOffloadingKind) const {
+  const Driver &D = getDriver();
+
----------------
I think this is only used once. We can probably move this down to near the place we need it, right?


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:427
 
+  // Forward last mtocdata/mno_tocdata options to -cc1
+  if (Args.hasArg(options::OPT_mtocdata_EQ, options::OPT_mno_tocdata_EQ,
----------------
End with period.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:428
+  // Forward last mtocdata/mno_tocdata options to -cc1
+  if (Args.hasArg(options::OPT_mtocdata_EQ, options::OPT_mno_tocdata_EQ,
+                  options::OPT_mtocdata, options::OPT_mno_tocdata)) {
----------------
- Can the TOC data option processing be a standalone static function?
- And for the static function, please accommodate any conditions that can be turned into early exits.
- Do we even need to check `options::OPT_mno_tocdata` (because if none of the other three are specified, it may not have an effect)?


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:433
+    // effect.
+    const bool TocDataInEffect = [&Args]() {
+      if (!Args.hasArg(options::OPT_mtocdata))
----------------
Add a comment that this checks how the global TOC attribute is set.
Or, we can update the variable name to include GlobalTOCAttribute, or TOCDataGloballyinEffect or something.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:437
+
+      const Arg *LastArg =
+          Args.getLastArg(options::OPT_mtocdata, options::OPT_mno_tocdata);
----------------
Add a null check for `LastArg`.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:444
+        (Args.getLastArgValue(options::OPT_mcmodel_EQ).equals("large") ||
+         Args.getLastArgValue(options::OPT_mcmodel_EQ).equals("medium"))) {
+      D.Diag(diag::warn_drv_unsupported_tocdata);
----------------
- I believe no braces are needed here.
- If this is moved into a new function, we can evaluate on whether this can be an early return.



================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:446
+      D.Diag(diag::warn_drv_unsupported_tocdata);
+    } else {
+      enum TOCDataOptionVal {
----------------
Add a comment on when and how we get into the `else` block.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:452
+
+      const TOCDataOptionVal ExplicitOptionDefaultOverride =
+          TocDataInEffect ? AddressInTOC : DataInTOC;
----------------
I think this can go away and explicitly check if the option type matches the `TocDataInEffect`.


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:455
+
+      llvm::StringSet<> ExplicitlySpecifiedGlobals;
+      for (const auto Arg : Args.filtered(options::OPT_mtocdata_EQ,
----------------
Add a comment to specify that we're going through the list of exceptions to the globals.
Also, can we add more comments throughout this function?


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:458
+                                          options::OPT_mno_tocdata_EQ)) {
+        TOCDataOptionVal OptionType =
+            Arg->getOption().matches(options::OPT_mtocdata_EQ) ? DataInTOC
----------------
- We may want to rename this so its more clearer.
- Possibly remove the enums?
- remove `ExplicitOptionDefaultOverride`.
- Check if the option matches  `TocDataInEffect`?


================
Comment at: clang/lib/Driver/ToolChains/AIX.cpp:462
+        for (const char *Val : Arg->getValues()) {
+          if (OptionType == ExplicitOptionDefaultOverride)
+            ExplicitlySpecifiedGlobals.insert(Val);
----------------
- Add a comment elaborating on all of this.
- Consider moving the `for` loops into the `if`/`else` so it will be clearer and so we are not checking for every value.
- Maybe we can change this to: Does the option type match the global value in effect, and then do the opposite?
  - Get rid of the override, and if it doesn't match the default, then we insert (to maintain a set of exceptions to the default)



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

https://reviews.llvm.org/D153907