[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 557659.
MaskRay added a comment.

test rebase


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c


Index: clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
===
--- clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
+++ clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
@@ -32,14 +32,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}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2107,12 +2107,11 @@
   const Driver  = TC.getDriver();
   const llvm::Triple  = TC.getTriple();
   // Default small data limitation is eight.
-  const char *SmallDataLimit = "8";
+  const char *SmallDataLimit = "0";
   // 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);
 }
@@ -2120,7 +2119,6 @@
  .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);
 }


Index: clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
===
--- clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
+++ clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
@@ -32,14 +32,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}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2107,12 +2107,11 @@
   const Driver  = TC.getDriver();
   const llvm::Triple  = TC.getTriple();
   // Default small data limitation is eight.
-  const char *SmallDataLimit = "8";
+  const char *SmallDataLimit = "0";
   // 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);
 }
@@ -2120,7 +2119,6 @@
  .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);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D152279#4638173 , @asb wrote:

> In D152279#4612099 , @craig.topper 
> wrote:
>
>> In D152279#4612087 , @MaskRay 
>> wrote:
>>
>>> I am still interested in moving this forward. What should be done here? If 
>>> the decision is to keep the current odd default 8 for 
>>> `toolchains::RISCVToolChain`, I guess I'll have to take the compromise as 
>>> making a step forward is better than nothing.
>>
>> On 1 RV64 CPU I tried in our RTL simulator, changing from 8 to 0 reduced 
>> dhrystone score by 2.7%. Using 16, or 32 gave the same score as 8. Reducing 
>> 8 to 4 improved the score by 0.5%.
>
> Thanks for testing - do you have a view one way or another on this default? 
> I'm somewhat torn personally.

Is this patch blocked by anything?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

In D152279#4612099 , @craig.topper 
wrote:

> In D152279#4612087 , @MaskRay wrote:
>
>> I am still interested in moving this forward. What should be done here? If 
>> the decision is to keep the current odd default 8 for 
>> `toolchains::RISCVToolChain`, I guess I'll have to take the compromise as 
>> making a step forward is better than nothing.
>
> On 1 RV64 CPU I tried in our RTL simulator, changing from 8 to 0 reduced 
> dhrystone score by 2.7%. Using 16, or 32 gave the same score as 8. Reducing 8 
> to 4 improved the score by 0.5%.

Thanks for testing - do you have a view one way or another on this default? I'm 
somewhat torn personally.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D152279#4612099 , @craig.topper 
wrote:

> In D152279#4612087 , @MaskRay wrote:
>
>> I am still interested in moving this forward. What should be done here? If 
>> the decision is to keep the current odd default 8 for 
>> `toolchains::RISCVToolChain`, I guess I'll have to take the compromise as 
>> making a step forward is better than nothing.
>
> On 1 RV64 CPU I tried in our RTL simulator, changing from 8 to 0 reduced 
> dhrystone score by 2.7%. Using 16, or 32 gave the same score as 8. Reducing 8 
> to 4 improved the score by 0.5%.

Thank you for sharing the benchmarks!

My view is that global pointer relaxation is an expert option that the user 
needs to tune (like that ld.lld doesn't default to `--relax-gp`).
People can create more articles about global pointer relaxation usage, and make 
the default out of the business of the driver.
If anyone tells me sdata/srodata/sbss adoption for other languages' compiler 
drivers, I'll definitely tell them not to copy the 0/8 complex rules in clang 
driver:)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

In D152279#4612087 , @MaskRay wrote:

> I am still interested in moving this forward. What should be done here? If 
> the decision is to keep the current odd default 8 for 
> `toolchains::RISCVToolChain`, I guess I'll have to take the compromise as 
> making a step forward is better than nothing.

On 1 RV64 CPU I tried in our RTL simulator, changing from 8 to 0 reduced 
dhrystone score by 2.7%. Using 16, or 32 gave the same score as 8. Reducing 8 
to 4 improved the score by 0.5%.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

I am still interested in moving this forward. What should be done here? If the 
decision is to keep the current odd default 8 for `toolchains::RISCVToolChain`, 
I guess I'll have to take the compromise as making a step forward is better 
than nothing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Thank you for chiming in. I disagree that we keep default=8 for embedded 
without understanding (a) why 8 provides values and (b) justifying that the 
value is significant enough for embedded to be different.

I think Alex's argument "I think we can generally expect more willingness for 
people targeting embedded systems to explore different compiler flags" is 
actually a +1 to decrease the complexity here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Shiva Chen via Phabricator via cfe-commits]

shiva0217 added a comment.

In D152279#4422887 , @MaskRay wrote:

> In D152279#4420312 , @asb wrote:
>
>> In D152279#4415974 , @MaskRay 
>> wrote:
>>
>>> However, RISC-V `-msmall-data-limit=` is probably a case warranting a 
>>> difference.
>>> The global pointer relaxation has a very limited value (benchmarked by 
>>> multiple parties, including a party which implemented this feature in the 
>>> GNU toolchain: even they can only say the optimization only applies to very 
>>> specific projects).
>>> The default value is confusing: as explained by the summary.
>>>
>>> I suspect that `-msmall-data-limit=8` is too conservative, maybe 16 would 
>>> be better, I don't know. I think global pointer relaxation users should 
>>> toggle this by themselves, not relying on `0` or `8` default decided by a 
>>> bunch of strange conditions.
>>
>> I don't disagree that the small data limit being 8 rather than something 
>> else doesn't seem to be particularly well motivated, but I understand that 
>> the case where the option does make a difference is on embedded targets (I 
>> think the data that was shared before was for SPEC, but could be wrong?). I 
>> think we can generally expect more willingness for people targeting embedded 
>> systems to explore different compiler flags,
>
> Agree.
>
>> but just matching gcc does feel like a better default. What do you think 
>> about keeping the default for bare metal targets?
>
> I am unsure we want to add the complexity and refrain from changing the 
> default for bare metal targets due to haunted graveyards 
> https://www.usenix.org/sites/default/files/conference/protected-files/srecon17americas_slides_reese.pdf
>
> We are already different from GCC in a number of ways:
>
> - for `-fpie`, we may emit `.sdata`/`.sbss` but GCC won't
> - we accept `-G` while GCC doesn't.
> - we don't emit `.srodata`.
>
> I made a comment on https://lists.riscv.org/g/sig-toolchains/message/619 and 
> informed GCC folks in case they can change the default as well.
> If they don't, I think we made a good choice for not following this 
> particular behavior.
>
> (I feel sad that I did not see D57497  and 
> it landed with a blocked review. If we started from scratch, we would 
> probably run into a cleaner state.)

Hi MaskRay,

-fpie is an oversight. Thanks for pointing out. For D57497 
, I thought it behave as good community 
citizen that trying to address all the comments.
Is there unaddressing comment? If there is, that might not be intended.

The patch tries to mimic the GCC to enable the relaxation. It generally put the 
data smaller than the threshold to the small data section which near gp and has 
higher possibility to transfer to gp based load/store.

Relaxation is less useful for large scale software. In previous time, most 
developers focus on embedded worlds. Now the flags become less friendly for the 
OS world.
I agree to change the default to make the OS world life easier.

I also agree with Alex. Should we preserve the threshold for embedded usage? 
Should we try to sync with GCC as possible?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D152279#4420312 , @asb wrote:

> In D152279#4415974 , @MaskRay wrote:
>
>> However, RISC-V `-msmall-data-limit=` is probably a case warranting a 
>> difference.
>> The global pointer relaxation has a very limited value (benchmarked by 
>> multiple parties, including a party which implemented this feature in the 
>> GNU toolchain: even they can only say the optimization only applies to very 
>> specific projects).
>> The default value is confusing: as explained by the summary.
>>
>> I suspect that `-msmall-data-limit=8` is too conservative, maybe 16 would be 
>> better, I don't know. I think global pointer relaxation users should toggle 
>> this by themselves, not relying on `0` or `8` default decided by a bunch of 
>> strange conditions.
>
> I don't disagree that the small data limit being 8 rather than something else 
> doesn't seem to be particularly well motivated, but I understand that the 
> case where the option does make a difference is on embedded targets (I think 
> the data that was shared before was for SPEC, but could be wrong?). I think 
> we can generally expect more willingness for people targeting embedded 
> systems to explore different compiler flags,

Agree.

> but just matching gcc does feel like a better default. What do you think 
> about keeping the default for bare metal targets?

I am unsure we want to add the complexity and refrain from changing the default 
for bare metal targets due to haunted graveyards 
https://www.usenix.org/sites/default/files/conference/protected-files/srecon17americas_slides_reese.pdf

We are already different from GCC in a number of ways:

- for `-fpie`, we may emit `.sdata`/`.sbss` but GCC won't
- we accept `-G` while GCC doesn't.
- we don't emit `.srodata`.

I made a comment on https://lists.riscv.org/g/sig-toolchains/message/619 and 
informed GCC folks in case they can change the default as well.
If they don't, I think we made a good choice for not following this particular 
behavior.

(I feel sad that I did not see D57497  and it 
landed with a blocked review. If we started from scratch, we would probably run 
into a cleaner state.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

In D152279#4415974 , @MaskRay wrote:

> However, RISC-V `-msmall-data-limit=` is probably a case warranting a 
> difference.
> The global pointer relaxation has a very limited value (benchmarked by 
> multiple parties, including a party which implemented this feature in the GNU 
> toolchain: even they can only say the optimization only applies to very 
> specific projects).
> The default value is confusing: as explained by the summary.
>
> I suspect that `-msmall-data-limit=8` is too conservative, maybe 16 would be 
> better, I don't know. I think global pointer relaxation users should toggle 
> this by themselves, not relying on `0` or `8` default decided by a bunch of 
> strange conditions.

I don't disagree that the small data limit being 8 rather than something else 
doesn't seem to be particularly well motivated, but I understand that the case 
where the option does make a difference is on embedded targets (I think the 
data that was shared before was for SPEC, but could be wrong?). I think we can 
generally expect more willingness for people targeting embedded systems to 
explore different compiler flags, but just matching gcc does feel like a better 
default. What do you think about keeping the default for bare metal targets?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D152279#4414574 , @hiraditya wrote:

> In D152279#4406896 , @MaskRay wrote:
>
>> In D152279#4405901 , @asb wrote:
>>
>>> One of the key things we've been discussing on this at the LLVM call is 
>>> that we probably want to keep the small data limit for embedded targets.
>>
>> It'd be useful to hear from some concrete embedded target users, whether 
>> they customize `-msmall-data-limit=`, whether they are happy with 
>> `-msmall-data-limit=8`, or whether they are just unaware of this threshold.
>>
>> If an embedded system typically customizes compiler driver options a lot, I 
>> think they can consider adding `-msmall-data-limit=` (with a value suitable 
>> for their use cases) in a configuration file 
>> , not 
>> letting their use cases dictate Linux systems.
>>
>> I putting up the patch partly came from my stance finding UX of this option 
>> is really confusing and misleading. I wish that the embedded target users 
>> can give me compelling arguments to keep `-msmall-data-limit=8` (and not 
>> another value).
>
> The default of 8 is probably to make it consistent with gcc. Here is text 
> from man gcc 
>
>   -msmall-data
>   -mlarge-data
>   When -mexplicit-relocs is in effect, static data is accessed
>   via gp-relative relocations.  When -msmall-data is used,
>   objects 8 bytes long or smaller are placed in a small data
>   area (the ".sdata" and ".sbss" sections) and are accessed via
>   16-bit relocations off of the $gp register.  This limits the
>   size of the small data area to 64KB, but allows the variables
>   to be directly accessed via a single instruction.
>   
>   The default is -mlarge-data.  With this option the data area
>   is limited to just below 2GB.  Programs that require more
>   than 2GB of data must use "malloc" or "mmap" to allocate the
>   data in the heap instead of in the program's data segment.
>   
>   When generating code for shared libraries, -fpic implies
>   -msmall-data and -fPIC implies -mlarge-data.

This GCC documentation is about the rx port. GCC's alpha and rx ports support 
`-msmall-data`, but RISC-V just says:

> -msmall-data-limit=n   Put global and static data smaller than n bytes into a 
> special section (on some targets).

Matching GCC behaviors give many benefits and Clang does this a lot, especially 
in cases where matching/not-matching doesn't make much difference (i.e. we 
don't need to have an opinion).
However, RISC-V `-msmall-data-limit=` is probably a case warranting a 
difference.
The global pointer relaxation has a very limited value (benchmarked by multiple 
parties, including a party which implemented this feature in the GNU toolchain: 
even they can only say the optimization only applies to very specific projects).
The default value is confusing: as explained by the summary.

I suspect that `-msmall-data-limit=8` is too conservative, maybe 16 would be 
better, I don't know. I think global pointer relaxation users should toggle 
this by themselves, not relying on `0` or `8` default decided by a bunch of 
strange conditions.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Aditya Kumar via Phabricator via cfe-commits]

hiraditya added a comment.

In D152279#4406896 , @MaskRay wrote:

> In D152279#4405901 , @asb wrote:
>
>> One of the key things we've been discussing on this at the LLVM call is that 
>> we probably want to keep the small data limit for embedded targets.
>
> It'd be useful to hear from some concrete embedded target users, whether they 
> customize `-msmall-data-limit=`, whether they are happy with 
> `-msmall-data-limit=8`, or whether they are just unaware of this threshold.
>
> If an embedded system typically customizes compiler driver options a lot, I 
> think they can consider adding `-msmall-data-limit=` (with a value suitable 
> for their use cases) in a configuration file 
> , not 
> letting their use cases dictate Linux systems.
>
> I putting up the patch partly came from my stance finding UX of this option 
> is really confusing and misleading. I wish that the embedded target users can 
> give me compelling arguments to keep `-msmall-data-limit=8` (and not another 
> value).

The default of 8 is probably to make it consistent with gcc. Here is text from 
man gcc 

  -msmall-data
  -mlarge-data
  When -mexplicit-relocs is in effect, static data is accessed
  via gp-relative relocations.  When -msmall-data is used,
  objects 8 bytes long or smaller are placed in a small data
  area (the ".sdata" and ".sbss" sections) and are accessed via
  16-bit relocations off of the $gp register.  This limits the
  size of the small data area to 64KB, but allows the variables
  to be directly accessed via a single instruction.
  
  The default is -mlarge-data.  With this option the data area
  is limited to just below 2GB.  Programs that require more
  than 2GB of data must use "malloc" or "mmap" to allocate the
  data in the heap instead of in the program's data segment.
  
  When generating code for shared libraries, -fpic implies
  -msmall-data and -fPIC implies -mlarge-data.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D152279#4405901 , @asb wrote:

> One of the key things we've been discussing on this at the LLVM call is that 
> we probably want to keep the small data limit for embedded targets.

It'd be useful to hear from some concrete embedded target users, whether they 
customize `-msmall-data-limit=`, whether they are happy with 
`-msmall-data-limit=8`, or whether they are just unaware this threshold.

If an embedded system typically customizes compiler driver options a lot, I 
think they can consider adding `-msmall-data-limit=` (with a value suitable for 
their use cases) in a configuration file 
, not letting 
their use cases judge Linux systems.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

One of the key things we've been discussing on this at the LLVM call is that we 
probably want to keep the small data limit for embedded targets.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Alex Bradbury via Phabricator via cfe-commits]

asb added a comment.

In D152279#4403940 , @phosek wrote:

> We're planning to default to `-msmall-data-limit=0` for Android and Fuchsia 
> so I'm supportive of this change because it means less complexity and fewer 
> differences between platforms.
>
> I think it would be worth bringing up this topic at the RISC-V LLVM sync-up 
> call tomorrow.

Sounds good, I'll put it on the agenda.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

We're planning to default to `-msmall-data-limit=0` for Android and Fuchsia so 
I'm supportive of this change because it means less complexity and fewer 
differences between platforms.

I think it would be worth bringing up this topic at the RISC-V LLVM sync-up 
call tomorrow.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2087
 // Not support linker relaxation for PIC.
-SmallDataLimit = "0";
 if (Args.hasArg(options::OPT_G)) {
   D.Diag(diag::warn_drv_unsupported_sdata);

The code is written in a confusing way with several problems including the 
unclear diagnostic. I just left a comment on 
https://reviews.llvm.org/D57497#4399715 and don't want to address them in this 
patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152279/new/

https://reviews.llvm.org/D152279

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D152279: [Driver] Default -msmall-data-limit= to 0

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: apazos, asb, craig.topper, hiraditya, jrtc27, 
shiva0217.
Herald added subscribers: luke, frasercrmck, luismarques, sameer.abuasal, 
s.egerton, Jim, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, 
edward-jones, zzheng, niosHD, sabuasal, simoncook, johnrusso, rbar.
Herald added a project: All.
MaskRay requested review of this revision.
Herald added subscribers: cfe-commits, pcwang-thead.
Herald added a project: clang.

D57497  added -msmall-data-limit= as an alias 
for -G and defaulted it to 8 for
-fno-pic/-fpie.

GCC documents this option as "Put global and static data smaller than  
bytes into a special section (on some targets)."
The targets are not documented, but it seems to not use the value for -fpie.

I think the different behavior for -fno-pic/-fpie/-fpic is not a good
idea and defaulting to -msmall-data-limit=0 makes more sense.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152279

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c


Index: clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
===
--- clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
+++ clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
@@ -28,14 +28,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}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2079,12 +2079,11 @@
   const Driver  = TC.getDriver();
   const llvm::Triple  = TC.getTriple();
   // Default small data limitation is eight.
-  const char *SmallDataLimit = "8";
+  const char *SmallDataLimit = "0";
   // 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);
 }
@@ -2092,7 +2091,6 @@
  .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);
 }


Index: clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
===
--- clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
+++ clang/test/CodeGen/RISCV/riscv-sdata-module-flag.c
@@ -28,14 +28,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}
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -2079,12 +2079,11 @@
   const Driver  = TC.getDriver();
   const llvm::Triple  = TC.getTriple();
   // Default small data limitation is eight.
-  const char *SmallDataLimit = "8";
+  const char *SmallDataLimit = "0";
   // 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);
 }
@@ -2092,7 +2091,6 @@
  .equals_insensitive("large") &&
  (Triple.getArch() == llvm::Triple::riscv64)) {
 // Not support linker relaxation for RV64 with large code model.
-SmallDataLimit = "0";
 if