Passing small data limitation to RISCVELFTargetObjectFile by module flag,
So the backend can set small data section threshold by the value.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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?
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).
- Setting small data limitation to zero for PIC and RV64 with large code model.
- Support passing small data limitation with LTO enabled.
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?
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
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
Support passing small data limitation for target riscv32-unknown-linux-gnu with LTO enabled.
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?
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.
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.
- 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.
- 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?
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?
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?
lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
1803 ↗ | (On Diff #190595) | 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. |
Update patch to address Ana's comments.
- Ignoring -G when -msmall-data-limit= in the command line and show the warning message
- Removing LTO plugin invoking which should introduce in another patch
lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
1813 ↗ | (On Diff #191640) | 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. |
lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
1813 ↗ | (On Diff #191640) | Hi Ana, |
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).
clang/docs/ClangCommandLineReference.rst | ||
---|---|---|
3202 | "(RISC-V only)" please | |
clang/lib/CodeGen/CodeGenModule.cpp | ||
689 | You should not be using ModFlagBehaviourFirstVal here - use Error if that is what you intend for conflicting values. The other possible option is Max but that could be confusing (I think we'd prefer Min, which doesn't yet exist) |
clang/docs/ClangCommandLineReference.rst | ||
---|---|---|
3202 | s/arg/limit/ s/limitation/limit/ | |
clang/include/clang/Basic/DiagnosticDriverKinds.td | ||
403 | Rather: "ignoring '-G' with '-msmall-data-limit'" | |
406 | Rather: "ignoring '-msmall-data-limit=' with '-mcmodel=large' for PIC or RV64" | |
clang/include/clang/Driver/CC1Options.td | ||
317 | s/limitation/limit/ | |
clang/include/clang/Driver/Options.td | ||
2304 | s/limitation/limit/ |
I believe my previous comments have indeed been addressed.
clang/docs/ClangCommandLineReference.rst | ||
---|---|---|
3202 | This seems like needless noise; it's already in the RISCV section, we don't need to say it's RISC-V only in the body too. Other architectures are inconsistent about this but I see it as a waste of time? |
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 created, no target feature is passed, and that no warnings are printed.
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.
Hi Ana,
Thanks for catching this, I'll fix it and add testing lines for it.
It seems that SDataLimit will be honored in LTO mode because getModuleMetadata will be called in AsmPrinter::doInitialization and SelectSectionForGlobal will be called from AsmPrinter::doFinalization->emitGlobalVariable->SectionForGLobal->SelectSecfionForGlobal.
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?
Hi Ana,
Thanks for trying the patch. msmall-data-limit is also a RISCV GCC flag, so I would recommend using the same flag as GCC.
How hard would it be to use the -msmall-data-threshold flag work if a -msmall-data-limit flag is not provided?
I think it won't be hard, users just need to find the same semantic flags when switching between LLVM and GCC. If we use the same flag for the same functionality, we could avoid the effort. Does it sound reasonable?
I'm not suggesting "don't support -msmall-data-limit=". I'm suggesting "use -msmall-data-threshold= if -msmall-data-limit= is not specified".
Actually, we can always add this support in later, it shouldn't block this patch. I'm happy for this patch to be merged - @jrtc27 is too, so let's wait for @apazos's final thoughts.
Hi @apazos,
I try to implement -small-data-limit alias of -small-data-threshold, it will trigger the assertion "Multi-level aliases are not supported." in Option constructor because -msmall-data-threshold alias to -G. So I implement -small-data-limit alias of -G, then we can get -msmall-data-limit= and -msmall-data-threshold= inputs by OPT_G in SetRISCVSmallDataLimit(). In a meanwhile, warning of "Ignore -G because -msmall-data-limit= has higher priority" will be removed and -G may override -msmall-data-limit if -G is the last flag. What do you think?
clang/lib/Driver/ToolChains/Clang.cpp | ||
---|---|---|
1970 | There are multiple issues. With -fpic -fpic, this will give a warning when -msmall-data-limit=N is specified, which is undesired. | |
1978 | -mcmodel=Large is rejected. We don't need to use equals_lower. | |
clang/test/CodeGen/riscv-sdata-module-flag.c | ||
1 | test/CodeGen tests usually use %clang_cc1, not %clang. Many tests here test the driver behavior and should be placed into test/Driver. |
"(RISC-V only)" please