[RISCV] Place mergeable small read only data into srodata section

[kito-cheng]

Small mergeable read only data was place on the sdata before, but it also means it lose the mergeable property, which means lose some code size optimization opportunity during link time.

[llvmbot]

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

Author: Kito Cheng (kito-cheng)

Changes

Small mergeable read only data was place on the sdata before, but it also means it lose the mergeable property, which means lose some code size optimization opportunity during link time.

---

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

3 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp (+24-2)
• (modified) llvm/lib/Target/RISCV/RISCVTargetObjectFile.h (+5)
• (added) llvm/test/CodeGen/RISCV/srodata.ll (+47)

diff --git a/llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp b/llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp
index 1535149b919b55..30abbd461cd236 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp
+++ b/llvm/lib/Target/RISCV/RISCVTargetObjectFile.cpp
@@ -32,6 +32,16 @@ void RISCVELFTargetObjectFile::Initialize(MCContext &Ctx,
       ".sdata", ELF::SHT_PROGBITS, ELF::SHF_WRITE | ELF::SHF_ALLOC);
   SmallBSSSection = getContext().getELFSection(".sbss", ELF::SHT_NOBITS,
                                                ELF::SHF_WRITE | ELF::SHF_ALLOC);
+  SmallRODataSection =
+      getContext().getELFSection(".srodata", ELF::SHT_PROGBITS, ELF::SHF_ALLOC);
+  SmallROData4Section = getContext().getELFSection(
+      ".srodata.cst4", ELF::SHT_PROGBITS, ELF::SHF_ALLOC | ELF::SHF_MERGE, 4);
+  SmallROData8Section = getContext().getELFSection(
+      ".srodata.cst8", ELF::SHT_PROGBITS, ELF::SHF_ALLOC | ELF::SHF_MERGE, 8);
+  SmallROData16Section = getContext().getELFSection(
+      ".srodata.cst16", ELF::SHT_PROGBITS, ELF::SHF_ALLOC | ELF::SHF_MERGE, 16);
+  SmallROData32Section = getContext().getELFSection(
+      ".srodata.cst32", ELF::SHT_PROGBITS, ELF::SHF_ALLOC | ELF::SHF_MERGE, 32);
 }
 
 const MCExpr *RISCVELFTargetObjectFile::getIndirectSymViaGOTPCRel(
@@ -126,8 +136,20 @@ bool RISCVELFTargetObjectFile::isConstantInSmallSection(
 MCSection *RISCVELFTargetObjectFile::getSectionForConstant(
     const DataLayout &DL, SectionKind Kind, const Constant *C,
     Align &Alignment) const {
-  if (isConstantInSmallSection(DL, C))
-    return SmallDataSection;
+  if (isConstantInSmallSection(DL, C)) {
+    if (Kind.isMergeableConst4())
+      return SmallROData4Section;
+    else if (Kind.isMergeableConst8())
+      return SmallROData8Section;
+    else if (Kind.isMergeableConst16())
+      return SmallROData16Section;
+    else if (Kind.isMergeableConst32())
+      return SmallROData32Section;
+    else
+      // LLVM only generate up to .rodata.cst32, and use .rodata section if more
+      // than 32 bytes, so just use .srodata here.
+      return SmallRODataSection;
+  }
 
   // Otherwise, we work the same as ELF.
   return TargetLoweringObjectFileELF::getSectionForConstant(DL, Kind, C,
diff --git a/llvm/lib/Target/RISCV/RISCVTargetObjectFile.h b/llvm/lib/Target/RISCV/RISCVTargetObjectFile.h
index 0910fbd3d95041..05e61ac874abba 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetObjectFile.h
+++ b/llvm/lib/Target/RISCV/RISCVTargetObjectFile.h
@@ -16,6 +16,11 @@ namespace llvm {
 /// This implementation is used for RISC-V ELF targets.
 class RISCVELFTargetObjectFile : public TargetLoweringObjectFileELF {
   MCSection *SmallDataSection;
+  MCSection *SmallRODataSection;
+  MCSection *SmallROData4Section;
+  MCSection *SmallROData8Section;
+  MCSection *SmallROData16Section;
+  MCSection *SmallROData32Section;
   MCSection *SmallBSSSection;
   unsigned SSThreshold = 8;
 
diff --git a/llvm/test/CodeGen/RISCV/srodata.ll b/llvm/test/CodeGen/RISCV/srodata.ll
new file mode 100644
index 00000000000000..1d5bd904f233fe
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/srodata.ll
@@ -0,0 +1,47 @@
+; RUN: sed 's/SMALL_DATA_LIMIT/0/g' %s | \
+; RUN:   llc -mtriple=riscv32 -mattr=+d | \
+; RUN:   FileCheck -check-prefix=CHECK-SDL-0 %s
+; RUN: sed 's/SMALL_DATA_LIMIT/0/g' %s | \
+; RUN:   llc -mtriple=riscv64 -mattr=+d | \
+; RUN:   FileCheck -check-prefix=CHECK-SDL-0 %s
+; RUN: sed 's/SMALL_DATA_LIMIT/4/g' %s | \
+; RUN:   llc -mtriple=riscv32 -mattr=+d | \
+; RUN:   FileCheck -check-prefix=CHECK-SDL-4 %s
+; RUN: sed 's/SMALL_DATA_LIMIT/4/g' %s | \
+; RUN:   llc -mtriple=riscv64 -mattr=+d | \
+; RUN:   FileCheck -check-prefix=CHECK-SDL-4 %s
+; RUN: sed 's/SMALL_DATA_LIMIT/8/g' %s | \
+; RUN:   llc -mtriple=riscv32 -mattr=+d | \
+; RUN:   FileCheck -check-prefix=CHECK-SDL-8 %s
+; RUN: sed 's/SMALL_DATA_LIMIT/8/g' %s | \
+; RUN:   llc -mtriple=riscv64 -mattr=+d | \
+; RUN:   FileCheck -check-prefix=CHECK-SDL-8 %s
+; RUN: sed 's/SMALL_DATA_LIMIT/16/g' %s | \
+; RUN:   llc -mtriple=riscv32 -mattr=+d | \
+; RUN:   FileCheck -check-prefix=CHECK-SDL-16 %s
+; RUN: sed 's/SMALL_DATA_LIMIT/16/g' %s | \
+; RUN:   llc -mtriple=riscv64 -mattr=+d | \
+; RUN:   FileCheck -check-prefix=CHECK-SDL-16 %s
+
+define dso_local float @foof() {
+entry:
+  ret float 0x400A08ACA0000000
+}
+
+define dso_local double @foo() {
+entry:
+  ret double 0x400A08AC91C3E242
+}
+
+!llvm.module.flags = !{!0}
+
+!0 = !{i32 8, !"SmallDataLimit", i32 SMALL_DATA_LIMIT}
+
+; CHECK-SDL-0-NOT:    .section        .srodata.cst4
+; CHECK-SDL-0-NOT:    .section        .srodata.cst8
+; CHECK-SDL-4:        .section        .srodata.cst4
+; CHECK-SDL-4-NOT:    .section        .srodata.cst8
+; CHECK-SDL-8:        .section        .srodata.cst4
+; CHECK-SDL-8:        .section        .srodata.cst8
+; CHECK-SDL-16:       .section        .srodata.cst4
+; CHECK-SDL-16:       .section        .srodata.cst8

[topperc]

Does gcc do something similar?

[kito-cheng]

Does gcc do something similar?

Yeah, here is the reference: https://github.com/gcc-mirror/gcc/blob/master/gcc/config/riscv/riscv.cc#L6177-L6189

[MaskRay]

I am still interested in changing the default limit from 8 to 0. Is there interest for GCC to do so?

[kito-cheng]

I am still interested in changing the default limit from 8 to 0. Is there interest for GCC to do so?

From the benchmarking aspect, it's hard to say I am interesting on change the default value honestly, since I didn't got the point about the disadvantage when the GP relaxation is turn off, but it did help when GP relaxation turn on, however I may missed something, so I am happy to discuss this more, and change my mind IF we gather enough arguments :)

	w/ GP relax	w/o GP relax
Default limit 0	Less chance on GP relax *1	No difference?*3
Default limit 8	More chance on GP relax *2	No difference

*1: because the range is limited, so same range cover more symbol have a higher winning rate.
*2: Compare to Default limit 0
*3: I guess I may missed something here, so need your input here.

[stephan-gh]

I believe this PR might fix the problem I reported on the LLVM security bug tracker back in January, since I was unsure about the security implications: https://bugs.chromium.org/p/llvm/issues/detail?id=61 It was decided that this is better handled publicly.

Aside from not being able to merge those constants the current behavior of placing constants in .sdata by default is also non-ideal because it makes it impossible to differentiate between small read-only constants and actual small mutable global variables. This is necessary if you want to favor security hardening (by mapping the constants read-only at runtime) over the slight performance/size benefit of linker relaxation. For example, in the Linux kernel a custom linker script is used to place .srodata next to .rodata rather than merging it into the mutable .data. This works only with GCC at the moment because LLVM does not generate .srodata.

In my report linked above, I suggested setting -msmall-data-limit to 0 by default as a workaround but these changes look like they would fix the actual problem. :)

[topperc]

This LGTM. @MaskRay does it look good to you? I think independent of the default small data limit we still need to fix this when there is a small data limit.

[ilovepi]

LGTM from my perspective, and I agree this seems likely to address the issue raised in https://bugs.chromium.org/p/llvm/issues/detail?id=61 . In Fuchsia, we set -msmall-data-limit=0 everywhere, in part due to that, but that isn't the only concern. We've set that for a long time, in part due to our use of the Shadow Call Stack, and some interactions w/ small data limit were not desirable. I'm not completely sure if those still exists, though. So I agree w/ @MaskRay 's suggestion about choosing a different default, though that is an orthogonal question.

[petrhosek]

LGTM this is beneficial for our embedded/baremetal use cases.

[kito-cheng]

Gonna merge, I believe it's an improvement, and default value of that is separated issue :)