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

[MaskRay]

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 ([RISCV] Place mergeable small read only data into srodata section #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.

[llvmbot]

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang

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

Author: Fangrui Song (MaskRay)

Changes

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.

---

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

5 Files Affected:

• (modified) clang/lib/Driver/ToolChains/Clang.cpp (+4-37)
• (modified) clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c (+2-2)
• (removed) clang/test/Driver/riscv-sdata-warning.c (-4)
• (added) clang/test/Driver/riscv-sdata.c (+5)
• (modified) llvm/lib/Target/RISCV/RISCVTargetObjectFile.h (+1-1)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 6e1b7e8657d0dc..9b9eab3990607f 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -2029,42 +2029,6 @@ void Clang::AddPPCTargetArgs(const ArgList &Args,
   }
 }
 
-static void SetRISCVSmallDataLimit(const ToolChain &TC, const ArgList &Args,
-                                   ArgStringList &CmdArgs) {
-  const Driver &D = TC.getDriver();
-  const llvm::Triple &Triple = TC.getTriple();
-  // Default small data limitation is eight.
-  const char *SmallDataLimit = "8";
-  // Get small data limitation.
-  if (Args.getLastArg(options::OPT_shared, options::OPT_fpic,
-                      options::OPT_fPIC)) {
-    // Not support linker relaxation for PIC.
-    SmallDataLimit = "0";
-    if (Args.hasArg(options::OPT_G)) {
-      D.Diag(diag::warn_drv_unsupported_sdata);
-    }
-  } else if (Args.getLastArgValue(options::OPT_mcmodel_EQ)
-                 .equals_insensitive("large") &&
-             (Triple.getArch() == llvm::Triple::riscv64)) {
-    // Not support linker relaxation for RV64 with large code model.
-    SmallDataLimit = "0";
-    if (Args.hasArg(options::OPT_G)) {
-      D.Diag(diag::warn_drv_unsupported_sdata);
-    }
-  } else if (Triple.isAndroid()) {
-    // GP relaxation is not supported on Android.
-    SmallDataLimit = "0";
-    if (Args.hasArg(options::OPT_G)) {
-      D.Diag(diag::warn_drv_unsupported_sdata);
-    }
-  } else if (Arg *A = Args.getLastArg(options::OPT_G)) {
-    SmallDataLimit = A->getValue();
-  }
-  // Forward the -msmall-data-limit= option.
-  CmdArgs.push_back("-msmall-data-limit");
-  CmdArgs.push_back(SmallDataLimit);
-}
-
 void Clang::AddRISCVTargetArgs(const ArgList &Args,
                                ArgStringList &CmdArgs) const {
   const llvm::Triple &Triple = getToolChain().getTriple();
@@ -2073,7 +2037,10 @@ void Clang::AddRISCVTargetArgs(const ArgList &Args,
   CmdArgs.push_back("-target-abi");
   CmdArgs.push_back(ABIName.data());
 
-  SetRISCVSmallDataLimit(getToolChain(), Args, CmdArgs);
+  if (Arg *A = Args.getLastArg(options::OPT_G)) {
+    CmdArgs.push_back("-msmall-data-limit");
+    CmdArgs.push_back(A->getValue());
+  }
 
   if (!Args.hasFlag(options::OPT_mimplicit_float,
                     options::OPT_mno_implicit_float, true))
diff --git a/clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c b/clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
index 9bd69e7995877e..e91b2e7f225f52 100644
--- a/clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
+++ b/clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
@@ -30,14 +30,14 @@
 
 void test(void) {}
 
-// RV32-DEFAULT: !{i32 8, !"SmallDataLimit", i32 8}
+// RV32-DEFAULT: !{i32 8, !"SmallDataLimit", i32 0}
 // RV32-G4:      !{i32 8, !"SmallDataLimit", i32 4}
 // RV32-S0:      !{i32 8, !"SmallDataLimit", i32 0}
 // RV32-S2G4:    !{i32 8, !"SmallDataLimit", i32 4}
 // RV32-T16:     !{i32 8, !"SmallDataLimit", i32 16}
 // RV32-PIC:     !{i32 8, !"SmallDataLimit", i32 0}
 
-// RV64-DEFAULT: !{i32 8, !"SmallDataLimit", i32 8}
+// RV64-DEFAULT: !{i32 8, !"SmallDataLimit", i32 0}
 // RV64-G4:      !{i32 8, !"SmallDataLimit", i32 4}
 // RV64-S0:      !{i32 8, !"SmallDataLimit", i32 0}
 // RV64-S2G4:    !{i32 8, !"SmallDataLimit", i32 4}
diff --git a/clang/test/Driver/riscv-sdata-warning.c b/clang/test/Driver/riscv-sdata-warning.c
deleted file mode 100644
index ace9a32ee116fd..00000000000000
--- a/clang/test/Driver/riscv-sdata-warning.c
+++ /dev/null
@@ -1,4 +0,0 @@
-// REQUIRES: riscv-registered-target
-// RUN: %clang -S --target=riscv32-unknown-elf -fpic -msmall-data-limit=8 %s 2>&1 \
-// RUN:   | FileCheck -check-prefix=CHECK-PIC-SDATA %s
-// CHECK-PIC-SDATA: warning: ignoring '-msmall-data-limit=' with -mcmodel=large for -fpic or RV64
diff --git a/clang/test/Driver/riscv-sdata.c b/clang/test/Driver/riscv-sdata.c
new file mode 100644
index 00000000000000..84cd109813db4c
--- /dev/null
+++ b/clang/test/Driver/riscv-sdata.c
@@ -0,0 +1,5 @@
+// RUN: %clang -### -S --target=riscv64 %s 2>&1 | FileCheck %s
+// RUN: %clang -### -S --target=riscv64 -msmall-data-limit=8 %s 2>&1 | FileCheck %s --check-prefix=EIGHT
+
+// CHECK-NOT: "-msmall-data-limit"
+// EIGHT: "-msmall-data-limit" "8"
diff --git a/llvm/lib/Target/RISCV/RISCVTargetObjectFile.h b/llvm/lib/Target/RISCV/RISCVTargetObjectFile.h
index 0910fbd3d95041..2d4fa211fa303e 100644
--- a/llvm/lib/Target/RISCV/RISCVTargetObjectFile.h
+++ b/llvm/lib/Target/RISCV/RISCVTargetObjectFile.h
@@ -17,7 +17,7 @@ namespace llvm {
 class RISCVELFTargetObjectFile : public TargetLoweringObjectFileELF {
   MCSection *SmallDataSection;
   MCSection *SmallBSSSection;
-  unsigned SSThreshold = 8;
+  unsigned SSThreshold = 0;
 
 public:
   unsigned getTextSectionAlignment() const override;

[MaskRay]

Ping:)

[kito-cheng]

There is some discussion in last (2024/2/29) LLVM sync up meeting: We all agree that might not useful in linux target and those platforms disable GP relaxation, like Android and fuchsia; However it's still useful for embedded toolchain, so this change may surprise those embedded toolchain user - cause some code size regression due to this change; although lld NOT default enable GP relaxation and also not implement all GP relaxation (R_RISCV_PCREL_HI20 + R_RISCV_PCREL_LO12_I or R_RISCV_PCREL_LO12_S is missing), but user may use GNU linker or proprietary linker, so people would like to keep some case using same default in some case.

---

Back to the implementation - yes, GCC is not support -G in this case, but why clang support -G? it's kinda inherit from the MIPS and Hexagon, they defined -msmall-data-threshold and then alias to -G, so RISC-V also follow the same convention: -msmall-data-limit= is alias to -G (that's my understanding after archaeology, although I work with Shiva at that timing but I don't have too much memory about that).

-mcmodel=large doesn't work for RISC-V yet, so the special case is strange.

My best guess is that is because @ShivaChen was working for Andes at that timing, and the patch is extracted from downstream source tree and it already has that logic, and no body notice that during review process...:P

[1] https://reviews.llvm.org/D57497

[MaskRay]

There is some discussion in last (2024/2/29) LLVM sync up meeting: We all agree that might not useful in linux target and those platforms disable GP relaxation, like Android and fuchsia;

Thanks for having the discussion. The motivation behind my revising this patch was the introduction of .srodata (#82214; sorry that I missed the discussion and I see that it landed now.), which would require different analysis from downstreams anyway.

However it's still useful for embedded toolchain, so this change may surprise those embedded toolchain user - cause some code size regression due to this change; although lld NOT default enable GP relaxation and also not implement all GP relaxation (R_RISCV_PCREL_HI20 + R_RISCV_PCREL_LO12_I or R_RISCV_PCREL_LO12_S is missing), but user may use GNU linker or proprietary linker, so people would like to keep some case using same default in some case.

This summarizes the situation well. However, I want to point out that the embedded toolchain users should probably use a configuration file for the -msmall-data-limit= customization. For quite a few options with controversial defaults, the desired problem-solving principle is to pick the simple code and let downstreams customize using a configuration file. This is a good opportunity to make this cleanup because .srodata requires size analysis anyway. If this helps them understand that the value of -msmall-data-limit= should be carefully evaluated, that'd be a bonus (the default, 8, might not work well either).

[MaskRay]

I really hope that we can fix the driver option issue. We should really consider that area is in flux (.srodata was introduced; there are upcoming -fdata-sections changes for .sbss). Fixing the default only now is indeed unfortunate and I am sorry for that, but there are sufficient changes in this area that downstream users will likely notice. Now is probably the best time to make such a change.

[lenary]

I was a reviewer on the original patch, and I can see that I let some not great things through - claiming options that should not have been claimed, etc.

I also do think that the situation has changed since then - more platform defaults are opting out of the sbss/sdata optimisation than opting into it, and the GP relaxation is now off-by-default in LLD.

I'm not sure how the default was chosen, or how representative it is, so I'm happy with this, as any toolchains can use configuration files for e.g. baremetal configurations. A lot of choice around the "right" value is going to heavily depend on the specific application, I expect (and probably also the word size, which hasn't been taken into account by this default anyway).

[MaskRay]

Thanks! I'll land this in a few days.

I believe it's now up to the gp default users to justify the default and why they cannot user a Clang configuration file (this is difficult to justify, as we've many many decisions to Clang configuration files). I think this is important to ensure that future sdata/sbss development like #87040 not cause disruption to users who don't use the feature.

[michaelmaitland]

Adding a data point here: changing the small data limit to 0 is causing significant regressions on dhrystone, spec2006, spec2017, and geekbench5 on the sifive-x280 and sifive-p470.

Separate from this data point, a default of 8 bytes may be justified because most scalar primitives are 8 bytes or smaller on most architectures (boolean, char, int, unsigned, long, float, double, half). Data larger than this is likely something like arrays or structs.