Remove small data limit for riscv64.*android triples

[hiraditya]

Summary

On Android GP register has been repurposed for SCS so there is no need to have .sdata section.

[MaskRay]

Could you add some justification for this change? It's fine to use .sdata in the absence of linker relaxation.
If Android doesn't want to see .sdata, then the motivation should be properly stated.

[craig.topper]

clang/lib/Driver/ToolChains/Clang.cpp

2088

I think you might need to force it to 0. It looks like it might default to 8 in the backend.

lib/Target/RISCV/RISCVTargetObjectFile.h
20:  unsigned SSThreshold = 8;

[jrtc27]

clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c

56

If this change does end up going in, this needs fixing

[pirama]

clang/lib/Driver/ToolChains/Clang.cpp

2088

CodeGenOpts sets the default to 0:

clang/include/clang/Basic/CodeGenOptions.def
/// The threshold to put data into small data section.
VALUE_CODEGENOPT(SmallDataLimit, 32, 0)

... which should then be covered by the test.

[craig.topper]

clang/lib/Driver/ToolChains/Clang.cpp

2088

Nevermind. I guess the CodeGenModule builds the module flag with 0 if we don't pass -msmall-data-limit to cc1.

[enh]

clang/lib/Driver/ToolChains/Clang.cpp

2087

do you mean "gp relaxation"?

that's what .sdata is used for (and nothing else), aiui?

[hiraditya]

clang/lib/Driver/ToolChains/Clang.cpp

2087

do you mean "gp relaxation"?

yes. i'll fix the comment.

[hiraditya]

clang/lib/Driver/ToolChains/Clang.cpp

2087

Other comments below use 'linker relaxation' btw. But i agree using 'gp relaxation' looks more precise.

[asb]

LGTM for what it's worth, but should get a LGTM from someone more on the Android or linker side.

[enh]

(sgtm from Android. we're reusing gp for SCS.)

[MaskRay]

In D151512#4391567, @MaskRay wrote:

Could you add some justification for this change? It's fine to use .sdata in the absence of linker relaxation.
If Android doesn't want to see .sdata, then the motivation should be properly stated.

Ping about this question.

[jrtc27]

clang/lib/Driver/ToolChains/Clang.cpp

2087

There are relaxations other than GP-based ones (which definitely still apply), so the specificity is important here. You're also missing an "is".

[MaskRay]

Another possible direction is to revert the decision to make -msmall-data-threshold=8 the default (D57497).
GP relaxation users can set the option explicitly, or use a configuration file (e.g. place riscv64-unknown-linux-gnu-clang.cfg beside clang) to set this value to be larger than 0.

-fPIC defaulting to 0 seems rather weird.

[jrtc27]

clang/lib/Driver/ToolChains/Clang.cpp

2099

Should you not be reusing this code and its behaviour?

/me notes this code is a bit of a mess, two duplicate blocks that should just be one block with a big condition...

[MaskRay]

In D151512#4399633, @MaskRay wrote:

Another possible direction is to revert the decision to make -msmall-data-threshold=8 the default (D57497).
GP relaxation users can set the option explicitly, or use a configuration file (e.g. place riscv64-unknown-linux-gnu-clang.cfg beside clang) to set this value to be larger than 0.

-fPIC defaulting to 0 seems rather weird.

Sent D152279 ([Driver] Default -msmall-data-limit= to 0) so that targets don't need to add more conditions of the default.

[hiraditya]

clang/lib/Driver/ToolChains/Clang.cpp

2099

Agreed that this function needs a rewrite.

[hiraditya]

In D151512#4399611, @MaskRay wrote:

In D151512#4391567, @MaskRay wrote:

Could you add some justification for this change? It's fine to use .sdata in the absence of linker relaxation.
If Android doesn't want to see .sdata, then the motivation should be properly stated.

Ping about this question.

Added in the commit description.

[hiraditya]

@jrtc27 let me know if you have pending feedback.

[jrtc27]

Same view as @MaskRay

[jrtc27]

That and you should probably be warning about -G being unused in the same way as for -fPIC and -mcmodel=large (whatever that is...) if this does make sense to do

[hiraditya]

I'll wait for the feedback from llvm-riscv call. If D152279 is okay to land then this patch can be abandoned.

[hiraditya]

That and you should probably be warning about -G being unused in the same way as for -fPIC and -mcmodel=large (whatever that is...) if this does make sense to do

Added warning

[hiraditya]

@jrtc27 Apart from @MaskRay 's comment if there isn't anything else I'd like to land this. Once the default setting of -msmall-data-limit=0 gets consolidated, this patch can be reverted easily.

[jrtc27]

clang/lib/Driver/ToolChains/Clang.cpp

2086

Don't do this

2109–2111

This would be more in keeping with the existing code (replaces the Android check above too, consolidating it all in one place). Maybe also better to have this as the first if in the chain so it's more robust against future changes to the logic above.

[hiraditya]

clang/lib/Driver/ToolChains/Clang.cpp

2109–2111

looks cleaner. updated

clang/lib/Driver/ToolChains/Clang.cpp

if (ABIName) {
     CmdArgs.push_back(ABIName);
   }
 }
 
 static void SetRISCVSmallDataLimit(const ToolChain &TC, const ArgList &Args,
                                    ArgStringList &CmdArgs) {
   const Driver &D = TC.getDriver();
   const llvm::Triple &Triple = TC.getTriple();
+
+  // Linker relaxation not supported on Android.
+  if (Triple.isAndroid())
+    return;
+
   // 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 (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();

clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c

 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -G4 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-G4
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -msmall-data-limit=0 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-S0
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -msmall-data-limit=2 -G4 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-S2G4
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -msmall-data-threshold=16 -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-T16
+// RUN: %clang -target riscv64-linux-android %s -S -emit-llvm -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64-ANDROID
+// RUN: %clang -target riscv64-linux-android %s -S -emit-llvm -msmall-data-limit=8 -o - \
+// RUN:   | FileCheck %s -check-prefix=RV64-ANDROID
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -fpic -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-PIC
 // RUN: %clang -target riscv64-unknown-elf %s -S -emit-llvm -mcmodel=large -o - \
 // RUN:   | FileCheck %s -check-prefix=RV64-LARGE
 
 void test(void) {}
 
 // RV32-DEFAULT: !{i32 8, !"SmallDataLimit", i32 8}

 // RV64-S2G4:    !{i32 8, !"SmallDataLimit", i32 4}
 // RV64-T16:     !{i32 8, !"SmallDataLimit", i32 16}
 // RV64-PIC:     !{i32 8, !"SmallDataLimit", i32 0}
 // RV64-LARGE:   !{i32 8, !"SmallDataLimit", i32 0}
 
 // The value will be passed by module flag instead of target feature.
 // RV32-S0-NOT: +small-data-limit=
 // RV64-S0-NOT: +small-data-limit=
+
+// RV64-ANDROID-NOT: small-data-limit
+// RV64-ANDROID:   !{i32 8, !"SmallDataLimit", i32 0}
 No newline at end of file