[AIX] Turn -fdata-sections on by default in Clang

[jasonliu]

Summary

This patch does the following:

1. Make InitTargetOptionsFromCodeGenFlags() accepts Triple as a parameter, because some options' default value is triple dependant.
2. DataSections is turned on by default on AIX for llc.
3. Clang Driver passes in -fdata-sections by default on AIX.
4. Test cases change accordingly because of the default behaviour change.

[jasonliu]

Not sure if there is a better way to do this. But I have to admit this approach along with D88493 is a bit fragile.
If plug-in writer forgets to set HasExplicitDataSections to true, then the final result that TargetMachine give for getDataSections() could be false on most platform even when they deliberately set DataSections to true.
We have other targets in LLVM that have -fdata-sections by default as well. And they do it differently as well, which makes this a bit more complicate to be consistent.
For example, cloudABI would pass -fdata-sections through the Clang Driver by default. But that approach means if some user just invoke llc directly, they could get the wrong default on llc for that platform.
Wasm would just overwrite the DataSections option to true in their TargetMachine. But that means you could not set it to false even if you want to.
I've thought about making the DataSections option in TargetOptions an enum instead of boolean value, where you could have Default, On and Off to represent what we actually want for the options. That's not a trivial change, as a lot of consumers for TargetOptions are relying it to be a boolean.
If you think it's a better way to solve this problem and I should explore, let me know.

[MaskRay]

clang/include/clang/Basic/CodeGenOptions.def

47 ↗

(On Diff #295820)

From the current code. I don't see HasExplicitDataSections is necessary. Please remove the lld changes.

clang/lib/Driver/ToolChains/Clang.cpp

4921

You can place AIX in isUseSeparateSections instead.

clang/lib/Frontend/CompilerInvocation.cpp

994 ↗

(On Diff #295820)

In many cases, there is no need for having both positive and negative CC1 options. The driver handles the default.

[jasonliu]

clang/include/clang/Basic/CodeGenOptions.def

47 ↗

(On Diff #295820)

The reason I need HasExplicitDataSections goes back to D88493(which I haven't land yet, because I actually need to land these two patches together to avoid build break). In D88493, I'm trying to get the llc's default for -data-sections to be correct for AIX and introduced HasExplicitDataSections. If HasExplicitDataSections is not set, then llc would think there is -data-sections set, so it would just take the target triple's default, which makes DataSections setting to be useless.
It seems there is no good way to set a certain TargetOption's default to be dependant on the current target triple. As I already mentioned in my own comment in this patch, one of the way to achieve that could be make DataSections an enum option in TargetOptions, so that it could have a Default enum which could mean true or false depending on the triple setting. But it could mean a bigger refactoring to this option.

[jasonliu]

clang/include/clang/Basic/CodeGenOptions.def

47 ↗

(On Diff #295820)

Correction:
If HasExplicitDataSections is not set, then llc would think there is no -data-sections set, so it would just take the target triple's default...

[MaskRay]

clang/include/clang/Basic/CodeGenOptions.def

47 ↗

(On Diff #295820)

D88748

[jasonliu]

An update follows up with the idea in D88748.

[MaskRay]

lld/Common/TargetOptionsCommandFlags.cpp

18

Currently lld does not need Triple. Consider not changing the signature of initTargetOptionsFromCodeGenFlags.

llvm/include/llvm/CodeGen/CommandFlags.h

144

If you are going to change the signature, consider renaming it to InitTargetOptionsFromCodeGenFlags to be consistent with the coding standard.

[DiggerLin]

clang/include/clang/Basic/CodeGenOptions.def

46 ↗

(On Diff #295820)

turn on by default , it should be changed to CODEGENOPT(DataSections , 1, 1) ?

[DiggerLin]

clang/include/clang/Basic/CodeGenOptions.def

46 ↗

(On Diff #295820)

please ignore above comment. DataSection is set by default only on AIX OS . the comment maybe need to be modified?

[DiggerLin]

clang/test/Driver/aix-data-sections.c

8

may be good to check the whether other OS platform behavior be changed or not?

[jasonliu]

clang/test/Driver/aix-data-sections.c

8

I think there are existing lit tests checked other platform's behavior.

linux platform behavior is tested in clang/test/Driver/function-sections.c
cloudABI platform behavior is tested in clang/test/Driver/cloudabi.c
msvc behavior is tested in clang/test/Driver/cl-options.c

lld/Common/TargetOptionsCommandFlags.cpp

18

This function acts like a forwarder for llvm::codegen::InitTargetOptionsFromCodeGenFlags.
I think it still makes sense to change the signature in this case to minimize the different variation of the function, as those variations cause confusion to people.
I will change the name to match the LLVM style.

[jasonliu]

Change lld's forwarder function's signature to match LLVM style.

[MaskRay]

lld/Common/TargetOptionsCommandFlags.cpp

18

If you want to change InitTargetOptionsFromCodeGenFlags in an incompatible way, you can by the way rename it to initTarget*.

lld functions should always stick with the camelCase rule. If there is no meaningful triple, I'd prefer leave out the parameter.

[jasonliu]

lld/Common/TargetOptionsCommandFlags.cpp

18

Sorry, didn't realize lld has this camelCase rule.
I don't have a better naming for it, so I will just leave the function signature untouched.

[MaskRay]

A less intrusive approach is to not touch the existing InitTargetOptionsFromCodeGenFlags without parameters. You can add an initTargetOptionsFromCodeGenFlags with const Tripe & as its parameter.

[jasonliu]

In D88737#2321921, @MaskRay wrote:

A less intrusive approach is to not touch the existing InitTargetOptionsFromCodeGenFlags without parameters. You can add an initTargetOptionsFromCodeGenFlags with const Tripe & as its parameter.

I thought about that. But would prefer people think about whether or not a proper Triple is actually needed in that scenario before they make a call to InitTargetOptionsFromCodeGenFlags.
Have an InitTargetOptionsFromCodeGenFlags without parameter, or has Triple() as default parameter could potentially results in people choose the wrong one to call because it's just more convenient.

[DiggerLin]

LGTM

[jasonliu]

Hi Fangrui(@MaskRay), are you okay with this patch to land as is?

[MaskRay]

LGTM.