This is an archive of the discontinued LLVM Phabricator instance.

Remove small data limit for riscv64.*android triples
ClosedPublic

Authored by hiraditya on May 25 2023, 3:42 PM.

Details

Summary

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

Diff Detail

Event Timeline

hiraditya created this revision.May 25 2023, 3:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 25 2023, 3:42 PM
hiraditya requested review of this revision.May 25 2023, 3:42 PM
pirama accepted this revision.Jun 2 2023, 12:25 PM
This revision is now accepted and ready to land.Jun 2 2023, 12:25 PM
MaskRay added a comment.EditedJun 2 2023, 12:31 PM

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 added inline comments.Jun 2 2023, 12:32 PM
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;
jrtc27 requested changes to this revision.Jun 2 2023, 12:32 PM
jrtc27 added inline comments.
clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
56

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

This revision now requires changes to proceed.Jun 2 2023, 12:32 PM
pirama added inline comments.Jun 2 2023, 12:36 PM
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.

craig.topper added inline comments.Jun 2 2023, 12:38 PM
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.

hiraditya updated this revision to Diff 528524.Jun 5 2023, 11:16 AM
hiraditya marked an inline comment as done.
enh added inline comments.Jun 5 2023, 11:24 AM
clang/lib/Driver/ToolChains/Clang.cpp
2082

do you mean "gp relaxation"?

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

hiraditya added inline comments.Jun 5 2023, 11:54 AM
clang/lib/Driver/ToolChains/Clang.cpp
2082

do you mean "gp relaxation"?

yes. i'll fix the comment.

hiraditya added inline comments.Jun 5 2023, 11:56 AM
clang/lib/Driver/ToolChains/Clang.cpp
2082

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

hiraditya updated this revision to Diff 528544.Jun 5 2023, 12:23 PM
asb added a comment.Jun 6 2023, 12:54 AM

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

enh accepted this revision.Jun 6 2023, 6:59 AM

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

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 added inline comments.Jun 6 2023, 8:00 AM
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.

jrtc27 added inline comments.Jun 6 2023, 8:02 AM
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...

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 updated this revision to Diff 528907.Jun 6 2023, 9:39 AM
hiraditya edited the summary of this revision. (Show Details)Jun 6 2023, 9:41 AM
hiraditya marked an inline comment as done.
hiraditya added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
2089

Agreed that this function needs a rewrite.

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.

@jrtc27 let me know if you have pending feedback.

jrtc27 added a comment.EditedJun 7 2023, 4:13 PM

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.

hiraditya updated this revision to Diff 530281.EditedJun 11 2023, 12:18 AM

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

@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 added inline comments.Jun 13 2023, 7:58 AM
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.

hiraditya updated this revision to Diff 530927.Jun 13 2023, 8:55 AM
hiraditya marked 2 inline comments as done.
hiraditya added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
2105–2107

looks cleaner. updated

This revision was not accepted when it landed; it landed in state Needs Review.Jun 14 2023, 11:17 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2023, 11:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript