Page MenuHomePhabricator

[RISCV] Passing small data limitation value to RISCV backend
Needs ReviewPublic

Authored by shiva0217 on Jan 30 2019, 9:48 PM.

Details

Reviewers
asb
apazos
jrtc27
Summary

Passing small data limitation to RISCVELFTargetObjectFile by module flag,
So the backend can set small data section threshold by the value.

Diff Detail

Repository
rC Clang

Event Timeline

shiva0217 created this revision.Jan 30 2019, 9:48 PM

As this mllvm option only affects the creation of ELF objects, do we also need to add a similar option for the LTO case, as the -G value would have no effect otherwise?

As this mllvm option only affects the creation of ELF objects, do we also need to add a similar option for the LTO case, as the -G value would have no effect otherwise?

Hi Simon, I think you're right, we may need a way to pass -G value for the LTO. One possible solution is adding passing code in AddGoldPlugin, but there's no other target specific code. I can't find other targets passing their cl::opt flags through the LTO. Any suggestions?

Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2019, 7:28 PM
apazos added a comment.Feb 2 2019, 6:45 PM

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 threshold value to the assembler (via -gpsize) and linker (via -G).

shiva0217 updated this revision to Diff 184936.Feb 3 2019, 1:38 AM
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. (Show Details)
  1. Setting small data limitation to zero for PIC and RV64 with large code model.
  2. Support passing small data limitation with LTO enabled.

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 threshold value to the assembler (via -gpsize) and linker (via -G).

Hi Ana, thanks for the tips!
I follow Hexagon and implement the passing code in RISCV::Linker::ConstructJob.
Could you help me to check whether it could work properly?

apazos added a comment.Feb 3 2019, 7:46 AM

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

apazos added a comment.Feb 3 2019, 8:11 AM

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

shiva0217 updated this revision to Diff 184988.Feb 3 2019, 6:44 PM

Support passing small data limitation for target riscv32-unknown-linux-gnu with LTO enabled.

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

Hi Ana, I've added the passing code in tools::gnutools::Linker::ConstructJob for non-baremetal target.
Thanks for pointing me out the missing part. :)

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?

apazos added a comment.Feb 5 2019, 3:33 PM

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 allowed to go into the small data section.
Simon, do you have any insight?
Shiva, maybe for now we don’t pass the flag to LTO. But I think you got the correct mechanism. The only other suggestion I have is to add a RISC-V specific function to avoid too much RISC-V specific code in gnutools::Linker::constructJob. You just check the triple and call something like toolchains::RISCVToolChain::AddGoldPluginAdditionalFlags.

jrtc27 requested changes to this revision.Feb 5 2019, 4:00 PM

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 COLLECT_GCC_OPTIONS. Finally, please add tests for -msmall-data-limit=... instead of or as well as -G, since GCC only recognises the former on RISC-V.

This revision now requires changes to proceed.Feb 5 2019, 4:00 PM
shiva0217 updated this revision to Diff 185491.Feb 5 2019, 11:31 PM
  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 main setting flag for RISCV GCC

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?

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 on/off without assigning a value.
For -mfpu=vfp4, each fpu configuration will be declared as a feature (e.g. FeatureVFP4) and the configurations are limited,
but the threshold value could be any number.
Did you mean declare as a target feature in RISCV.td or I misunderstanding something?

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 global into a specific section. Maybe worth sending an email to llvmdev to discuss the right way to represent this in IR?

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 global into a specific section. Maybe worth sending an email to llvmdev to discuss the right way to represent this in IR?

Hi Eli,
I have sent the email to llvmdev http://lists.llvm.org/pipermail/llvm-dev/2019-February/130222.html. It seems that there're not much consensus on how to represent in IR. I incline to implement passing through -plugin-opt= as the first version. We could create an incremental patch when we have more consensus on IR approach. What do you think?

shiva0217 updated this revision to Diff 190595.Mar 14 2019, 4:09 AM

Passing small data limitation by module flag.

Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2019, 4:09 AM
apazos added inline comments.Mar 20 2019, 2:05 PM
lib/Driver/ToolChains/Clang.cpp
1804

Might be simpler to just set it to 0, and if G is present in the command line, and the other flags are not present, then you change the value. Also, might be good to print a warning to let user know -G is ignored?

lib/Driver/ToolChains/RISCVToolchain.cpp
117 ↗(On Diff #190595)

This change is not part of this patch.

shiva0217 marked an inline comment as done.
shiva0217 edited the summary of this revision. (Show Details)
shiva0217 changed the repository for this revision from rL LLVM to rC Clang.
shiva0217 removed a project: Restricted Project.
shiva0217 removed a subscriber: llvm-commits.

Update patch to address Ana's comments.

  1. Ignoring -G when -msmall-data-limit= in the command line and show the warning message
  2. Removing LTO plugin invoking which should introduce in another patch
apazos added inline comments.Wed, Mar 27, 1:56 PM
lib/Driver/ToolChains/Clang.cpp
1813

Why do you we need to set a default? It will cause the optimization to be on now, and I thought we want to first prioritize the globals we want to consider for the optimization.
I think you should check for msmall-data-limit flag occurring with G, fpic and mcmodel flags and print warning in all cases.

shiva0217 updated this revision to Diff 192618.Thu, Mar 28, 5:53 AM

Add warning message for -msmall-data-limit with -fpic or RV64 with -mcmodel=large

shiva0217 marked an inline comment as done.Thu, Mar 28, 6:39 AM
shiva0217 added inline comments.
lib/Driver/ToolChains/Clang.cpp
1813

Hi Ana,
It seems that RISC-V GCC will set 8 as default value even with -mno-relax.
Another issue is that the sorting scope will be a compile unit. So when linker link the .sdata from objects, it will get globals from object A with usage times from high to low and then the globals from object B with usage times from high to low which probably not we want. The sorting might make sense when the LTO enabled.
I have added the warning message, thanks for the suggestion.

Jim added a subscriber: Jim.Mon, Apr 8, 1:30 AM