Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[AIX] [TOC] Add -mtocdata/-mno-tocdata options on AIX
Needs ReviewPublic

Authored by syzaara on Jun 27 2023, 11:58 AM.



This patch enables support that the XL compiler had for AIX under -qdatalocal/-qdataimported.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
cebowleratibm added inline comments.Jul 18 2023, 7:55 AM
7136 ↗(On Diff #541525)

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.

12 ↗(On Diff #541525)

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.

alexgatea added inline comments.Jul 18 2023, 9:21 AM
277 ↗(On Diff #540140)

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.

283 ↗(On Diff #540140)

Yes, that's right.

7136 ↗(On Diff #541525)

Good point! I'll change it

12 ↗(On Diff #541525)

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

hubert.reinterpretcast added inline comments.
96 ↗(On Diff #541525)

It seems the current state of the implementation may warrant a warning when using toc-data with the large code model. The feature is only effective if TOC-based relocations are used to refer to the toc-data symbol. Has that been implemented?

Addressed latest review comments

Added diagnostic for when -mtocdata is used with -mcmodel=large

1–8 ↗(On Diff #541647)

Use -### for driver test to:

  1. Avoid invocation of further components.
  2. Observe the -cc1 options being passed.

Aside from this driver test for the warning, this patch does not seem to have driver tests for validating that the option is passed to -cc1 if and only if it should be. The missing tests should be added.

555 ↗(On Diff #541647)

Typo fix.

7122–7124 ↗(On Diff #541647)

When emitting this message, the option should truly be ignored (e.g., driver should not pass the option to -cc1). Also, -mcmodel=medium is mapped by the driver to -mcmodel=large for AIX targets. The warning (and corresponding tests) should be extended to cover that.

1–8 ↗(On Diff #541647)

Aside from this driver test for the warning, this patch does not seem to have driver tests for validating that the option is passed to -cc1 if and only if it should be. The missing tests should be added.

Okay, I see test/Driver/toc-conf.c now. It looks like the patch was not generated with a consistent root directory?

alexgatea updated this revision to Diff 541703.Jul 18 2023, 1:22 PM

Addressed review comments


Clang IR codegen tests do not require the LLVM target to be enabled.


Same comment.


Same comment.


No need for -S with test based on -###.


What do you mean by "global variable"? Do internal linkage variables at global scope count? Do variables in named C++ namespaces (or static data members of classes) count? How are they to be named by the options?

I guess block-scope variables with static storage duration don't count.

Whether they count or not, the effect of applying "blanket" -mtocdata upon such variables should be tested.

cebowleratibm added inline comments.Jul 19 2023, 7:10 AM

You expect to see j in the IR, just that it shouldn't have the toc-data attribute. Perhaps it's better to check the positive (with the #0 omitted) because the check-not may be satisfied if there is a subtle change in how j is emitted. This comment applies to multiple instances.


nit: these read easier if the checks appear in the same order as the RUN steps above.


I assume you're checking that the -mno-tocdata property is passed on the cc1 invocation. This would read better if the cc1 were part of the match.


I guess block-scope variables with static storage duration don't count.

For the IBM XL compiler, block-scope variables with static storage duration do become toc-data when they are not part of the static pool (e.g., block-scope static in a C++ inline function).


This use of getGlobalIdentifier (having a user-interface that relies on its format) seems novel. It warrants updating the Doxygen description of the function.

I believe some discussion should be had on LLVM Discourse about this.

alexgatea updated this revision to Diff 542058.Jul 19 2023, 9:07 AM

Addressed some review comments, moved parsing of mtocdata options to AIX.cpp

cebowleratibm added inline comments.Jul 19 2023, 1:09 PM
274 ↗(On Diff #541525)

@hubert.reinterpretcast and I were discussing whether or not this query is the right query to use because you're string comparing the value against a string provided by the user on the command line. I think it's probably safer to use a well-defined linkage name for the comparison and set it up such that we don't unnecessarily compute it unless the toc-data feature is opted into.

We should discuss offline and update this thread with our conclusion.

alexgatea updated this revision to Diff 542202.Jul 19 2023, 2:45 PM

Fixed diff discrepancy, removed asserts for structs/arrays/vectors, updated doc

cebowleratibm added inline comments.Jul 19 2023, 2:49 PM

I think clang::DeclarationName::getAsIdentifierInfo would be reasonable way to do it. This is a facility for printing user readable names and it makes sense to me that it would be the string a user would expect to use when specifying the command line option.

If the variable isn't a simple name then it isn't eligible for toc-data.

The more desirable way to to this would be a source-level attribute, which is something that ought to be considered in conjunction with this work, though I understand this is primarily to have an equivalent of the XL -qdatalocal= option for AIX.

alexgatea updated this revision to Diff 542208.Jul 19 2023, 2:56 PM

Add back the missing tests


clang::DeclarationName::getAsIdentifierInfo sounds like it would not take scopes and linkage into account. It also sounds like it has additional complications around macros.

Limiting the ability to specify specific symbols to cases of external linkage using the mangled symbol names is perhaps the least controversial in terms of having something that is solid (albeit restrictive).

syzaara commandeered this revision.Jul 25 2023, 10:52 AM
syzaara added a reviewer: alexgatea.
lei added a subscriber: lei.Aug 1 2023, 7:40 AM
lei added inline comments.

nit: indent

MaskRay added inline comments.Aug 1 2023, 9:01 AM

If -mtocdata= is opt-in, the convention is to make just -mtocdata= CC1Option, but clear the flag for -mno-tocdata=.


StringSet has non-deterministic iteration order. Ensure that the behavior doesn't change :)

syzaara updated this revision to Diff 546098.Aug 1 2023, 9:45 AM
syzaara added a reviewer: hubert.reinterpretcast.

Addressed review comments.

amyk added a subscriber: amyk.Aug 16 2023, 8:02 AM

Initial review regarding some typos.

amyk added a comment.Aug 16 2023, 9:32 AM

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

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


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

Add double quotes for options.


Just say that it's only supported for small code model.


Can we move this under the option on 2957 (put positive and negative options together) and ensure the HelpTexts are updated accordingly?


I think this is only used once. We can probably move this down to near the place we need it, right?


End with period.

  • 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)?

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.


Add a null check for LastArg.

  • 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.

Add a comment on when and how we get into the else block.


I think this can go away and explicitly check if the option type matches the TocDataInEffect.


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?

  • We may want to rename this so its more clearer.
  • Possibly remove the enums?
  • remove ExplicitOptionDefaultOverride.
  • Check if the option matches TocDataInEffect?
  • 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)
amyk added a comment.Aug 16 2023, 9:34 AM

One last comment from the group code review.


What is inside the if/else are similar. If they're the same, maybe we can set a variable inside the if/else and then move the other lines out of the if/else.

syzaara set the repository for this revision to rG LLVM Github Monorepo.Aug 28 2023, 9:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2023, 9:23 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
syzaara updated this revision to Diff 554461.Aug 29 2023, 12:37 PM

@syzaara, I see the comment has been addressed but it is not marked as "done". The author is responsible for marking comments as "done" in Phabricator. Please mark all comments that you believe have been addressed as "done" so that other parties know if the comment is still pending action.


The "toc-data attribute" is an implementation detail. The user-facing documentation should not talk about it.


Please document the conditions for a variable to be "suitable".


The use of "all" is incorrect. Please qualify with "suitable" (and address this issue elsewhere in the text where applicable).

syzaara marked 30 inline comments as done.Aug 31 2023, 1:45 PM
syzaara added inline comments.Sep 28 2023, 6:35 AM

Our situation with these options is a bit more complicated because -mno-tocdata= is not just removing the entries that were specified by -mtocdata=
For example if we have:

-mtocdata=X -mno-tocdata=X -mtocdata=Y,Z,S

Then we could potentially just forward to CC1:


Rather than what we currently forward:

-mno-tocdata -mtocdata=Y,Z,S

However, if we have

-mtocdata -mno-tocdata=Y,Z,S

This means all suitable variables by default will become toc-data aside from the exceptions in -mno-tocdata=Y,Z,S
and we would need to forward both these options.

So the way we handle it is by passing to CC1 either

-mtocdata -mno-tocdata=


-mno-tocdata -mtocdata=

which is the current default setting along with the exceptions that need to be applied to that default setting.

DiggerLin added inline comments.

do we need CLOption, DXCOption here ? these two option is for Window OS. ?