[RISCV] Place mergeable small read only data into srodata section #82214
Conversation
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.
|
@llvm/pr-subscribers-backend-risc-v Author: Kito Cheng (kito-cheng) ChangesSmall 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:
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
|
|
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 |
|
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 :)
|
|
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 |
There was a problem hiding this comment.
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.
|
Gonna merge, I believe it's an improvement, and default value of that is separated issue :) |
D57497 added -msmall-data-limit= as an alias for -G and defaulted it to 8 for -fno-pic/-fpie. The behavior is already different from GCC in a few ways: * GCC doesn't accept -G. * GCC -fpie doesn't seem to use -msmall-data-limit=. * GCC emits .srodata.cst* that we don't use (#82214). Writable contents caused confusion (https://bugs.chromium.org/p/llvm/issues/detail?id=61) In addition, * claiming `-shared` means we don't get a desired `-Wunused-command-line-argument` for `clang --target=riscv64-linux-gnu -fpic -c -shared a.c`. * -mcmodel=large doesn't work for RISC-V yet, so the special case is strange. * It's quite unusual to emit a warning when an option (unrelated to relocation model) is used with -fpic. * We don't want future configurations (Android) to continue adding customization to `SetRISCVSmallDataLimit`. I believe the extra code just doesn't pull its weight and should be cleaned up. This patch also changes the default to 0. GP relaxation users are encouraged to specify these customization options explicitly. Pull Request: #83093
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.