Page MenuHomePhabricator

[RISCV] Passing small data limitation value to RISCV backend
ClosedPublic

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

Details

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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.Mar 27 2019, 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.Mar 28 2019, 5:53 AM

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

shiva0217 marked an inline comment as done.Mar 28 2019, 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.Apr 8 2019, 1:30 AM
apazos added a comment.Mar 9 2020, 3:46 PM

Shiva, we forgot about this patch. Can you rebase it so we move on with merging.

Shiva, we forgot about this patch. Can you rebase it so we move on with merging.

Ok, I'll rebase the patch.

shiva0217 updated this revision to Diff 249279.Mar 9 2020, 10:51 PM

Rebase to the trunk

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
2958 ↗(On Diff #249279)

"(RISC-V only)" please

clang/lib/CodeGen/CodeGenModule.cpp
688 ↗(On Diff #249279)

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)

shiva0217 updated this revision to Diff 249336.Mar 10 2020, 5:53 AM

Update patch to address @lenary's comments.

evandro added inline comments.Mar 10 2020, 12:07 PM
clang/docs/ClangCommandLineReference.rst
2958 ↗(On Diff #249336)
s/arg/limit/
s/limitation/limit/
clang/include/clang/Basic/DiagnosticDriverKinds.td
403 ↗(On Diff #249336)

Rather:

"ignoring '-G' with '-msmall-data-limit'"
406 ↗(On Diff #249336)

Rather:

"ignoring '-msmall-data-limit=' with '-mcmodel=large' for PIC or RV64"
clang/include/clang/Driver/CC1Options.td
317 ↗(On Diff #249336)
s/limitation/limit/
clang/include/clang/Driver/Options.td
2302 ↗(On Diff #249336)
s/limitation/limit/
shiva0217 updated this revision to Diff 249545.Mar 10 2020, 7:46 PM

Update patch to address @evandro's comments.

I believe my previous comments have indeed been addressed.

clang/docs/ClangCommandLineReference.rst
2958 ↗(On Diff #249279)

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?

Update patch to address @jrtc27's 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 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.

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.

Hi Ana,
Thanks for catching this, I'll fix it and add testing lines for it.

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.

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.

Update the patch to address @apazos's comments.

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?

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.

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?

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?

lenary accepted this revision.Mar 16 2020, 3:02 AM

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.

Shiva, how about making the flag small-data-limit alias of -msmall-data-threshold?

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 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?

shiva0217 updated this revision to Diff 250984.Mar 17 2020, 9:11 PM

Update patch to address @apazos's comments.

lenary accepted this revision.Mar 18 2020, 8:14 AM

Thanks Shiva, making it alias of -G makes sense, LGTM.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 19 2020, 8:18 PM
This revision was automatically updated to reflect the committed changes.