Page MenuHomePhabricator

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

Authored by jasonliu on Oct 2 2020, 7:00 AM.

Details

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.

Diff Detail

Event Timeline

jasonliu created this revision.Oct 2 2020, 7:00 AM
jasonliu requested review of this revision.Oct 2 2020, 7:00 AM

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 requested changes to this revision.Oct 2 2020, 8:34 AM
MaskRay added inline comments.
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.

This revision now requires changes to proceed.Oct 2 2020, 8:34 AM
jasonliu added inline comments.Oct 2 2020, 9:17 AM
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 added inline comments.Oct 2 2020, 9:35 AM
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 added inline comments.Oct 2 2020, 10:34 AM
clang/include/clang/Basic/CodeGenOptions.def
47 ↗(On Diff #295820)
jasonliu planned changes to this revision.Oct 2 2020, 5:01 PM
jasonliu updated this revision to Diff 295950.Oct 2 2020, 8:00 PM

An update follows up with the idea in D88748.

Herald added a project: Restricted Project. · View Herald TranscriptOct 2 2020, 8:00 PM
jasonliu edited the summary of this revision. (Show Details)Oct 2 2020, 8:03 PM
jasonliu marked an inline comment as done.
MaskRay added inline comments.Oct 3 2020, 3:14 PM
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 added inline comments.Oct 5 2020, 8:01 AM
clang/include/clang/Basic/CodeGenOptions.def
46 ↗(On Diff #295820)

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

DiggerLin added inline comments.Oct 5 2020, 12:57 PM
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 added inline comments.Oct 6 2020, 6:17 AM
clang/test/Driver/aix-data-sections.c
8

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

jasonliu marked an inline comment as done.Oct 7 2020, 7:08 AM
jasonliu added inline comments.
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 updated this revision to Diff 296665.Oct 7 2020, 7:12 AM

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

jasonliu removed a subscriber: lldb-commits.
MaskRay added inline comments.Oct 7 2020, 11:16 PM
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 updated this revision to Diff 296965.Oct 8 2020, 7:12 AM
jasonliu added inline comments.
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.

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

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.

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

MaskRay accepted this revision.Oct 13 2020, 8:33 AM

LGTM.

This revision is now accepted and ready to land.Oct 13 2020, 8:33 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2020, 8:58 AM