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

[Alex Gatea via Phabricator via llvm-commits]

alexgatea added inline comments.


================
Comment at: lib/CodeGen/Targets/PPC.cpp:277
+    // Is this a global variable specified by the user as toc-data?
+    bool UserSpecifiedTOC =
+        llvm::binary_search(M.getCodeGenOpts().TocDataVarsUserSpecified, GVId);
----------------
cebowleratibm wrote:
> There are some code paths where you don't need to evaluate the explicit tocdata/no-tocdata if it would have been redundant to the default already specified so you could be more efficient.
This is true about UserSpecifiedNotTOC but we need to determine UserSpecifiedTOC even if we compile with -mtocdata because we only emit diagnostics for variables which are explicitly specified as toc-data.


================
Comment at: lib/CodeGen/Targets/PPC.cpp:283
+    assert(
+        (!UserSpecifiedTOC || !UserSpecifiedNotTOC) &&
+        "The same variable cannot be marked as both TocData and NotTocData.");
----------------
cebowleratibm wrote:
> Is this to say that the driver sets the cc1 call to ensure no variable shows up in both lists?
Yes, that's right.


================
Comment at: lib/Driver/ToolChains/Clang.cpp:7136
+                                                             : AddressInTOC;
+      if (OptionType == ExplicitOptionDefaultOverride) {
+        for (const char *Val : Arg->getValues())
----------------
cebowleratibm wrote:
> unless there's a performance reason for keeping the `if` out of the loop, the code reads a little tighter if you push the if inside a single for loop.
> 
> I don't feel strongly but thought I'd point it out.  Use your discretion.
Good point! I'll change it


================
Comment at: test/CodeGen/PowerPC/toc-data-attribute.c:12
+int *i;
+int __attribute__((aligned(128))) j = 0;
+float k = 100.00;
----------------
cebowleratibm wrote:
> `j` is overaligned to be toc data.  Are you expecting a diagnostic or for it to be quietly handled notoc-data?
> 
> I suggest splitting out the symbols that can't be toc data to another test and check for a diagnostic.  If they're warnings then I suggest also checking that the IR did not apply the toc-data attribute.
I am expecting a diagnostic. I initially added another test verifying the diagnostics for symbols that can't be toc-data; I will add it back.
The idea was to separate the test for diagnostics and the test for the toc-data attribute (and lack of). Let me know if you disagree


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

https://reviews.llvm.org/D153907