On Android GP register has been repurposed for SCS so there is no need to have .sdata section.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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.
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2083 | 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; |
clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c | ||
---|---|---|
56 | If this change does end up going in, this needs fixing |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2083 | 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. |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2083 | Nevermind. I guess the CodeGenModule builds the module flag with 0 if we don't pass -msmall-data-limit to cc1. |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2082 | do you mean "gp relaxation"? that's what .sdata is used for (and nothing else), aiui? |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2082 |
yes. i'll fix the comment. |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2082 | Other comments below use 'linker relaxation' btw. But i agree using 'gp relaxation' looks more precise. |
LGTM for what it's worth, but should get a LGTM from someone more on the Android or linker side.
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2082 | There are relaxations other than GP-based ones (which definitely still apply), so the specificity is important here. You're also missing an "is". |
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.
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2089 | 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... |
Sent D152279 ([Driver] Default -msmall-data-limit= to 0) so that targets don't need to add more conditions of the default.
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2089 | Agreed that this function needs a rewrite. |
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
I'll wait for the feedback from llvm-riscv call. If D152279 is okay to land then this patch can be abandoned.
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
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2081 | Don't do this | |
2105–2107 | 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. |
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
2105–2107 | looks cleaner. updated |
Don't do this