[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

MaskRay added inline comments. Herald added subscribers: jobnoorman, luke, pcwang-thead, eopXD, VincentWu, vkmr, frasercrmck, arichardson. Herald added a project: All. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1970 + // Get small data limitation. + if (Args.getLastArg(

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGfc3752665f4b: [RISCV] Passing small data limitation value to RISCV backend (authored by shiva0217). Changed prior to comm

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. Thanks Shiva, making it alias of -G makes sense, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 ___ cfe-commits mailing list cfe-

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 250984. shiva0217 added a comment. Update patch to address @apazos's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 Files: clang/docs/ClangCommandLineRef

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1927881 , @apazos wrote: > Shiva, how about making the flag small-data-limit alias of > -msmall-data-threshold? Hi @apazos, I try to implement -small-data-limit alias of -small-data-threshold, it will trigger the a

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. Shiva, how about making the flag small-data-limit alias of -msmall-data-threshold? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 ___

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

lenary accepted this revision. lenary added a comment. In D57497#1923615 , @shiva0217 wrote: > In D57497#1921497 , @lenary wrote: > > > How hard would it be to use the `-msmall-data-threshold` flag work if a > > `-

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1921497 , @lenary wrote: > In D57497#1920870 , @shiva0217 wrote: > > > In D57497#1920097 , @apazos wrote: > > > > > Thanks Shiva, I res-y

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

lenary added a comment. In D57497#1920870 , @shiva0217 wrote: > In D57497#1920097 , @apazos wrote: > > > Thanks Shiva, I res-ynced and rebuilt the patch. It is working fine. > > > > I see there is a msmall-data-thre

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1920097 , @apazos wrote: > Thanks Shiva, I res-ynced and rebuilt the patch. It is working fine. > > I see there is a msmall-data-threshold flag used by Mips and Hexagon, and now > we are adding a new flag msmall-data-l

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. Thanks Shiva, I res-ynced and rebuilt the patch. It is working fine. I see there is a msmall-data-threshold flag used by Mips and Hexagon, and now we are adding a new flag msmall-data-limit. Should't we reuse the flag? Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 249835. shiva0217 added a comment. Update the patch to address @apazos's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 Files: clang/docs/ClangCommandLin

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1917835 , @apazos wrote: > Shiva, I see a warning always being printed: > > '+small-data-limit=' is not a recognized feature for this target (ignoring > feature) > > > This is because it is being passed down as a

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. Shiva, I am not sure how the SDataLimit is being honored in LTO mode. Where does getModuleMetadata get called? If the SelectSectionForGlobal api is called without getModuleMetadata being called first, it will use the default 8 instead of honoring the SDataLimit. Reposit

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. Shiva, I see a warning always being printed: '+small-data-limit=' is not a recognized feature for this target (ignoring feature) This is because it is being passed down as a target feature. Might be good to add a test case to make sure the SmallDataLimit module flag is

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 249558. shiva0217 added a comment. Update patch to address @jrtc27's comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 Files: clang/docs/ClangCommandLineRefe

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

jrtc27 added a comment. I believe my previous comments have indeed been addressed. Comment at: clang/docs/ClangCommandLineReference.rst:2958 + +Put global and static data smaller than the limitation into a special section (RISCV only) + lenary wrote: > "(RISC-

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 249545. shiva0217 added a comment. Update patch to address @evandro's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 Files: clang/docs/ClangCommandLineRe

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

evandro added inline comments. Comment at: clang/docs/ClangCommandLineReference.rst:2958 + +Put global and static data smaller than the limitation into a special section (RISC-V only) + ``` s/arg/limit/ s/limitation/limit/ ``` Comment at: clan

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 249336. shiva0217 added a comment. Update patch to address @lenary's comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 Files: clang/docs/ClangCommandLineRef

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

lenary added a comment. Two nits, then I'd be happy for this to land. It would be useful if @jrtc27 would also re-review, as he has blocking comments (which I believe are now addressed). Comment at: clang/docs/ClangCommandLineReference.rst:2958 + +Put global and static data s

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 249279. shiva0217 added a comment. Rebase to the trunk Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 Files: clang/docs/ClangCommandLineReference.rst clang/inclu

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1913473 , @apazos wrote: > Shiva, we forgot about this patch. Can you rebase it so we move on with > merging. Ok, I'll rebase the patch. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. Herald added subscribers: evandro, luismarques, sameer.abuasal, pzheng, s.egerton, lenary. Shiva, we forgot about this patch. Can you rebase it so we move on with merging. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://r

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 marked an inline comment as done. shiva0217 added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:1813 +} + } else if (Arg *A = Args.getLastArg(options::OPT_G)) { +SmallDataLimit = A->getValue(); apazos wrote: > Why do you we need

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 192618. shiva0217 added a comment. Add warning message for -msmall-data-limit with -fpic or RV64 with -mcmodel=large Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 Files: docs/C

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added inline comments. Comment at: lib/Driver/ToolChains/Clang.cpp:1813 +} + } else if (Arg *A = Args.getLastArg(options::OPT_G)) { +SmallDataLimit = A->getValue(); Why do you we need to set a default? It will cause the optimization to be on now,

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 191640. shiva0217 marked an inline comment as done. shiva0217 edited the summary of this revision. shiva0217 changed the repository for this revision from rL LLVM to rC Clang. shiva0217 removed a project: LLVM. shiva0217 removed a subscriber: llvm-commits. sh

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added inline comments. Herald added subscribers: benna, psnobl. Comment at: lib/Driver/ToolChains/Clang.cpp:1803 + + // Forward the -msmall-data-limit= option. + if (Arg *A = Args.getLastArg(options::OPT_G)) { Might be simpler to just set it to 0, and if

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 190595. shiva0217 added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Passing small data limitation by module flag. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1394137 , @efriedma wrote: > > Did you mean declare as a target feature in RISCV.td or I misunderstanding > > something? > > That's sort of the right idea, but I don't think it works in this context > because we aren'

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

efriedma added a comment. > Did you mean declare as a target feature in RISCV.td or I misunderstanding > something? That's sort of the right idea, but I don't think it works in this context because we aren't trying to change the generated code for a function; we actually need to stick the glob

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1388350 , @apazos wrote: > If this is a target flag in GCC, shouldn't we make it a LLVM Target feature > and pass it as -mattr, just like done for mrelax? Hi Ana, It seems that most of the -mattr features only obtain

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. If this is a target flag in GCC, shouldn't we make it a LLVM Target feature and pass it as -mattr, just like done for mrelax? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 185491. shiva0217 added a comment. 1. Remove passing path for LTO because Eli raised the concern that whether it would appropriate to assign the same limitation for all files when LTO enabled. 2. Add -msmall-data-limitation= as James pointed out it's the ma

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

jrtc27 requested changes to this revision. jrtc27 added a comment. This revision now requires changes to proceed. Please update `docs/ClangCommandLineReference.rst`. Also, in my limited testing, GCC just seems to pass the `-msmall-data-limit=...` flag straight through to the linker through `COLL

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. So Eli is concerned we might end up with many globals in the small data section or not picking the best candidates if we pass -G to all files in LTO. I don’t know if anyone has experimented with a heuristic to selectively pick which globals and of which size will be allow

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

efriedma added a comment. How do we actually want this to work for LTO? Would it make sense to encode this on global variables somehow, to allow different thresholds for different source files? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://r

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1382393 , @apazos wrote: > I don't see -plugin-opt=-riscv-ssection-threshold=.. being passed. > tools::gnutools::Linker::ConstructJob is being invoked with target > riscv32-unknown-linux-gnu > It has to work for risc

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 184988. shiva0217 added a comment. Support passing small data limitation for target riscv32-unknown-linux-gnu with LTO enabled. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57497/new/ https://reviews.llvm.org/D57497 File

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. I don't see -plugin-opt=-riscv-ssection-threshold=.. being passed. tools::gnutools::Linker::ConstructJob is being invoked with target riscv32-unknown-linux-gnu It has to work for riscv32-unknown-linux-gnu and riscv32-unknown-elf Repository: rC Clang CHANGES SINCE LAST

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

apazos added a comment. Hi Shiva, I will check, but I think you need to also modify gnutools:Linker because riscv::Linker is called for baremetal. I think you need in both places. The way I check is by invoking -flto -v from clang and look at the arguments passed to the compiler and linker Rep

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 added a comment. In D57497#1382206 , @apazos wrote: > Hi Shiva, I think you need to check for and pass along the -G option to the > linker (gnutools::Linker and RISCV::Linker) and will be available for LTO. > Check Hexagon, it passes the thres

[PATCH] D57497: [RISCV] Passing small data limitation value to RISCV backend

shiva0217 updated this revision to Diff 184936. shiva0217 retitled this revision from "[RISCV] Passing -G value to RISCV backend" to "[RISCV] Passing small data limitation value to RISCV backend". shiva0217 edited the summary of this revision. shiva0217 added a comment. 1. Setting small data limi