[RISCV] Let -data-sections also work on sbss/sdata sections #87040

KaiYG · 2024-03-29T07:40:44Z

Add an unique suffix to .sbss/.sdata if -fdata-sections.
Without assigning an unique .sbss/.sdata section to each symbols, a linker may not be able to remove unused part when gc-section since all used and unused symbols are all mixed in the same .sbss/.sdata section.
I believe this also matches the behavior of gcc.

…sections Copied from sdata-limit-8.ll and update the CHECKs, otherwise it matches any .sbss/.sdata regardless having a suffix or not.

Without assigning an unique .sbss/.sdata section to each symbols, a linker may not be able to remove unused part when gc-section since all used and unused symbols are all mixed in the same .sbss/.sdata section. I believe this also matches the behavior of gcc.

github-actions · 2024-03-29T07:41:01Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2024-03-29T07:41:28Z

@llvm/pr-subscribers-backend-risc-v

Author: KaiWeng (KaiYG)

Changes

Add an unique suffix to .sbss/.sdata if -fdata-sections.
Without assigning an unique .sbss/.sdata section to each symbols, a linker may not be able to remove unused part when gc-section since all used and unused symbols are all mixed in the same .sbss/.sdata section.
I believe this also matches the behavior of gcc.

Full diff: https://github.com/llvm/llvm-project/pull/87040.diff

2 Files Affected:

(modified) llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp (+30-4)
(added) llvm/test/CodeGen/RISCV/sdata-sections.ll (+17)

diff --git a/llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp b/llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp
index e7c1a7e5d8bca1..b13df905249ab5 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp
@@ -104,10 +104,36 @@ bool RISCVELFTargetObjectFile::isGlobalInSmallSection(
 MCSection *RISCVELFTargetObjectFile::SelectSectionForGlobal(
     const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
   // Handle Small Section classification here.
-  if (Kind.isBSS() && isGlobalInSmallSection(GO, TM))
-    return SmallBSSSection;
-  if (Kind.isData() && isGlobalInSmallSection(GO, TM))
-    return SmallDataSection;
+  if (isGlobalInSmallSection(GO, TM)) {
+    // Emit a unique section for sdata/sbss when -fdata-section.
+    bool EmitUniquedSection = TM.getDataSections();
+
+    if (Kind.isBSS()) {
+      if (EmitUniquedSection) {
+        StringRef Prefix(".sbss");
+        SmallString<128> Name(Prefix);
+        Name.append(".");
+        Name.append(GO->getName());
+        return getContext().getELFSection(Name.str(), ELF::SHT_NOBITS,
+                                          ELF::SHF_WRITE | ELF::SHF_ALLOC);
+      }
+
+      return SmallBSSSection;
+    }
+
+    if (Kind.isData()) {
+      if (EmitUniquedSection) {
+        StringRef Prefix(".sdata");
+        SmallString<128> Name(Prefix);
+        Name.append(".");
+        Name.append(GO->getName());
+        return getContext().getELFSection(Name.str(), ELF::SHT_PROGBITS,
+                                          ELF::SHF_WRITE | ELF::SHF_ALLOC);
+      }
+
+      return SmallDataSection;
+    }
+  }
 
   // Otherwise, we work the same as ELF.
   return TargetLoweringObjectFileELF::SelectSectionForGlobal(GO, Kind, TM);
diff --git a/llvm/test/CodeGen/RISCV/sdata-sections.ll b/llvm/test/CodeGen/RISCV/sdata-sections.ll
new file mode 100644
index 00000000000000..175e6d744843a8
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/sdata-sections.ll
@@ -0,0 +1,17 @@
+; RUN: llc -mtriple=riscv32 -data-sections < %s | FileCheck -check-prefix=RV32 %s
+; RUN: llc -mtriple=riscv64 -data-sections < %s | FileCheck -check-prefix=RV64 %s
+
+; Append an unique name to each sdata/sbss section when -data-section.
+
+@v = dso_local global i32 0, align 4
+@r = dso_local global i64 7, align 8
+
+; SmallDataLimit set to 8, so we expect @v will be put in sbss
+; and @r will be put in sdata.
+!llvm.module.flags = !{!0}
+!0 = !{i32 8, !"SmallDataLimit", i32 8}
+
+; RV32:    .section        .sbss.v,"aw"
+; RV32:    .section        .sdata.r,"aw"
+; RV64:    .section        .sbss.v,"aw"
+; RV64:    .section        .sdata.r,"aw"

KaiYG · 2024-06-12T02:24:22Z

Ping.
I think this patch will save code size on both bfd/lld linker, and a similar patch has been done on the Hexagon backend.

kito-cheng

LGTM :)

jonathonpenix · 2024-08-13T21:22:54Z

Are there any outstanding concerns on this patch? We recently saw this issue and I was wondering if it would be ok to merge this at this point. Thanks!

topperc · 2024-08-15T15:36:00Z

Do we need to update this code in lld/ELF/Writer.cpp to handle the suffix?

  if (config->emachine == EM_RISCV) {                                            
    // .sdata and .sbss are placed closer to make GP relaxation more profitable  
    // and match GNU ld.                                                         
    StringRef name = osec.name;                                                  
    if (name == ".sdata" || (osec.type == SHT_NOBITS && name != ".sbss"))        
      rank |= 1;                                                                 
  }

Probably should be a separate patch.

lenary · 2024-08-15T15:38:06Z

@topperc yeah, I was thinking that too - I'll take a closer look.

RISC-V GCC with `-fdata-sections` will emit `.sbss.<name>` and `.sdata.<name>` sections for small data items. We intend to add the same behaviour to Clang/LLVM in llvm#87040. This change ensures that any section starting `.sbss` is put in an output section called `.sbss`, and the same for `.sdata`, so that the RISC-V specific code for allocating an output order for `.sbss` and `.sdata` sections can apply. I did not add a `.sdata.rel.ro` prefix to the list as clang/llvm does not seem to create these at all. Whether it should be or not is a different matter.

MaskRay

Before we do more work with sdata/sbss I think we should consider
#83093

or at least ensure regular non-baremetal targets don't get weird non-zero -msmall-data-limit=

RISC-V GCC with `-fdata-sections` will emit `.sbss.<name>`, `.srodata.<name>`, and `.sdata.<name>` sections for small data items of different kinds. Clang/LLVM already emits `.srodata.*` sections, and we intend to emit the other two section name patterns in llvm#87040. This change ensures that any input sections starting `.sbss` are combined into one output section called `.sbss`, and the same respectively for `.srodata` and `.sdata`. This also allows the existing RISC-V specific code for determining an output order for `.sbss` and `.sdata` sections to apply to placing the sections.

llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp

RISC-V GCC with `-fdata-sections` will emit `.sbss.<name>`, `.srodata.<name>`, and `.sdata.<name>` sections for small data items of different kinds. Clang/LLVM already emits `.srodata.*` sections, and we intend to emit the other two section name patterns in llvm#87040. This change ensures that any input sections starting `.sbss` are combined into one output section called `.sbss`, and the same respectively for `.srodata` and `.sdata`. This also allows the existing RISC-V specific code for determining an output order for `.sbss` and `.sdata` sections to apply to placing the sections.

RISC-V GCC with `-fdata-sections` will emit `.sbss.<name>`, `.srodata.<name>`, and `.sdata.<name>` sections for small data items of different kinds. Clang/LLVM already emits `.srodata.*` sections, and we intend to emit the other two section name patterns in #87040. This change ensures that any input sections starting `.sbss` are combined into one output section called `.sbss`, and the same respectively for `.srodata` and `.sdata`. This also allows the existing RISC-V specific code for determining an output order for `.sbss` and `.sdata` sections to apply to placing the sections.

petrhosek · 2024-08-19T17:38:54Z

There's a change in semantics which I think is undesirable due to https://github.com/llvm/llvm-project/pull/87040/files#diff-16406b22032eaeb81527fc5cf1938dd0981c926fcad2f5b2e9a9f9db7d31f03cR79-R82

For example, if you have the following:

__attribute__((section(".bss"))) int foo = 0;

foo will end up in .bss even when -fdata-sections is set.

However, if you have the following:

__attribute__((section(".sbss"))) int foo = 0;

foo will end up in .sbss.foo after this change and I think that's undesirable.

If a symbol has an explicit section, we should honor that. This could be achieved simply by always returning false if https://github.com/llvm/llvm-project/pull/87040/files#diff-16406b22032eaeb81527fc5cf1938dd0981c926fcad2f5b2e9a9f9db7d31f03cL76 is true.

KaiYG · 2024-08-20T10:09:40Z

There's a change in semantics which I think is undesirable due to https://github.com/llvm/llvm-project/pull/87040/files#diff-16406b22032eaeb81527fc5cf1938dd0981c926fcad2f5b2e9a9f9db7d31f03cR79-R82

Good catch. But I patched this issue in another way, as I think isGlobalInSmallSection should still return true for explicit sdata/sbss sections, and EmitUniquedSection should be set to false instead.

lenary

I re-looked at this code and there are still issues with globals with explicit sections

llvm/test/CodeGen/RISCV/sdata-sections.ll

llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp

lenary

LGTM

llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp

lenary · 2024-08-22T18:48:43Z

@MaskRay ping?

lenary · 2024-08-23T11:35:47Z

@KaiYG right now, if i try to squash merge, the commit will use your @users.noreply.github.com email address, which LLVM doesn't want in the commit history - please can you follow the instructions here: Setting your commit email address on GitHub to change it to your andestech email? Then I will squash/merge.

KaiYG · 2024-08-23T13:03:58Z

@lenary thanks for the heads up! I've changed my email settings and it should show my public email now.

lenary · 2024-08-23T13:06:25Z

Thanks, that did the right thing :)

github-actions · 2024-08-23T13:06:33Z

@KaiYG Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

Add an unique suffix to .sbss/.sdata if -fdata-sections. Without assigning an unique .sbss/.sdata section to each symbols, a linker may not be able to remove unused part when gc-section since all used and unused symbols are all mixed in the same .sbss/.sdata section. I believe this also matches the behavior of gcc.

lenary · 2024-08-27T11:39:40Z

I forgot that a question about backporting this change came up in the RISC-V sync-up. I think we're too late to backport this to 19.x, as it isn't a bugfix, it's an improvement.

KaiYG added 2 commits March 29, 2024 15:05

[RISCV] Pre-commit a test showing apply -data-sections on sbss/sdata …

26180ed

…sections Copied from sdata-limit-8.ll and update the CHECKs, otherwise it matches any .sbss/.sdata regardless having a suffix or not.

llvmbot added the backend:RISC-V label Mar 29, 2024

dtcxzyw requested review from MaskRay, jrtc27, topperc and wangpc-pp March 29, 2024 10:17

kito-cheng approved these changes Jun 12, 2024

View reviewed changes

lenary mentioned this pull request Aug 15, 2024

[lld][ELF] Combine uniqued small data sections #104485

Merged

MaskRay requested changes Aug 15, 2024

View reviewed changes

MaskRay mentioned this pull request Aug 17, 2024

[Driver] Default -msmall-data-limit= to 0 and clean up code #83093

Merged

petrhosek reviewed Aug 19, 2024

View reviewed changes

llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp Outdated Show resolved Hide resolved

petrhosek reviewed Aug 19, 2024

View reviewed changes

llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp Outdated Show resolved Hide resolved

Polish the usage of StringRef

816d447

Honor explicit sdata/sbss sections

763aa11

lenary reviewed Aug 21, 2024

View reviewed changes

llvm/test/CodeGen/RISCV/sdata-sections.ll Show resolved Hide resolved

llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp Show resolved Hide resolved

Add tests with non-sdata/sbss custom section names

67945a4

lenary approved these changes Aug 22, 2024

View reviewed changes

llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp Show resolved Hide resolved

petrhosek approved these changes Aug 22, 2024

View reviewed changes

MaskRay approved these changes Aug 22, 2024

View reviewed changes

lenary merged commit 4d348f7 into llvm:main Aug 23, 2024
8 checks passed

[RISCV] Let -data-sections also work on sbss/sdata sections #87040

[RISCV] Let -data-sections also work on sbss/sdata sections #87040

Uh oh!

Conversation

KaiYG commented Mar 29, 2024

Uh oh!

github-actions bot commented Mar 29, 2024

Uh oh!

llvmbot commented Mar 29, 2024

Uh oh!

KaiYG commented Jun 12, 2024

Uh oh!

kito-cheng left a comment

Choose a reason for hiding this comment

Uh oh!

jonathonpenix commented Aug 13, 2024

Uh oh!

topperc commented Aug 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lenary commented Aug 15, 2024

Uh oh!

MaskRay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

petrhosek commented Aug 19, 2024

Uh oh!

KaiYG commented Aug 20, 2024

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lenary commented Aug 22, 2024

Uh oh!

lenary commented Aug 23, 2024

Uh oh!

KaiYG commented Aug 23, 2024

Uh oh!

Uh oh!

lenary commented Aug 23, 2024

Uh oh!

github-actions bot commented Aug 23, 2024

Uh oh!

lenary commented Aug 27, 2024

Uh oh!

Uh oh!

topperc commented Aug 15, 2024 •

edited

Loading