Page MenuHomePhabricator

[RISCV] Put data smaller than eight bytes to small data section
ClosedPublic

Authored by shiva0217 on Jan 30 2019, 6:30 PM.

Details

Summary

Because of gp = sdata_start_address + 0x800, gp with signed twelve-bit offset could covert most of the small data section. Linker relaxation could transfer the multiple data accessing instructions to a gp base with signed twelve-bit offset instruction.

Diff Detail

Repository
rL LLVM

Event Timeline

shiva0217 created this revision.Jan 30 2019, 6:30 PM
apazos added inline comments.Feb 1 2019, 4:48 PM
lib/Target/RISCV/RISCVTargetObjectFile.cpp
39 ↗(On Diff #184434)

It seems we force a default value (4 or 8) even when the threshold flag is off (value is 0)?
So how to disable the optimization?

Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 4:48 PM
shiva0217 updated this revision to Diff 184875.Feb 1 2019, 5:33 PM

Fix the issue for -G0 as Ana pointed out.

shiva0217 marked an inline comment as done.Feb 1 2019, 5:37 PM
shiva0217 added inline comments.
lib/Target/RISCV/RISCVTargetObjectFile.cpp
39 ↗(On Diff #184434)

Hi Ana,
You're right. I've updated the revision to address the issue.
Thanks for the catch.

shiva0217 updated this revision to Diff 184891.Feb 1 2019, 9:40 PM
shiva0217 retitled this revision from [RISCV] Put data smaller than integer type to small data section to [RISCV] Put data smaller than eight bytes to small data section.

Correct the default small data limitation to 8 bytes for RV32 and RV64.
Setting the limitation to 0 for PIC.
The test case didn' add the testing line for PIC because we don't support pic yet, it will trigger "Unable to lowerGlobalAddress" error message.

shiva0217 updated this revision to Diff 184923.Feb 2 2019, 5:45 PM

Add testing for large code model.

shiva0217 updated this revision to Diff 184935.Feb 3 2019, 1:30 AM

Remove guard condition for large code model which will implement in clang.

shiva0217 updated this revision to Diff 190594.Mar 14 2019, 4:07 AM

Add getModuleMetadata() to read SmallDataLimit Module flag.

apazos added inline comments.Mar 20 2019, 1:46 PM
lib/Target/RISCV/RISCVTargetObjectFile.cpp
103 ↗(On Diff #190594)

Here the flag value is overwritten by the module setting, so how can we use the flag?

shiva0217 marked an inline comment as done.Mar 20 2019, 11:32 PM
shiva0217 added inline comments.
lib/Target/RISCV/RISCVTargetObjectFile.cpp
103 ↗(On Diff #190594)

We probably could remove riscv-ssection-threshold flag because front end won't pass the flag and emit the threshold by module flag.
Thanks for the catch.

Remove riscv-ssection-threshold flag because front end will pass the threshold by module flag.

efriedma added inline comments.
lib/Target/RISCV/RISCVTargetObjectFile.cpp
17 ↗(On Diff #191639)

This isn't thread-safe.

63 ↗(On Diff #191639)

We never put variables with internal linkage into a small data section? That seems a little weird.

apazos added inline comments.Mar 28 2019, 5:01 PM
lib/Target/RISCV/RISCVTargetObjectFile.cpp
17 ↗(On Diff #191639)

We can put it in RISCVELFTargetObjectFile just like SmallDataSection and SmallBSSSection

shiva0217 marked 2 inline comments as done.Mar 29 2019, 2:32 AM
shiva0217 added inline comments.
lib/Target/RISCV/RISCVTargetObjectFile.cpp
17 ↗(On Diff #191639)

Hi Eli and Ana, thanks for the guidance, I'll put SSThreshold in RISCVELFTargetObjectFile.

63 ↗(On Diff #191639)

We should handle internal linkage as well, thanks for the catch.

shiva0217 updated this revision to Diff 192789.Mar 29 2019, 2:39 AM

Update patch to address the comments from Eli and Ana.

Jim added a subscriber: Jim.Apr 8 2019, 1:30 AM
asb accepted this revision.Apr 10 2019, 10:50 AM

Many thanks for this. I've added some minor formatting and comment phrasing nits, but otherwise this is looking good to me.

lib/CodeGen/AsmPrinter/AsmPrinter.cpp
260 ↗(On Diff #192789)

clang-format prefers const_cast<TargetLoweringObjectFile &>

lib/Target/RISCV/RISCVTargetObjectFile.cpp
91 ↗(On Diff #192789)

Style: should be auto &MFE : ModuleFlags

test/CodeGen/RISCV/sdata-limit-0.ll
7 ↗(On Diff #192789)

"so we expect no data will be put in sbss or sdata."

test/CodeGen/RISCV/sdata-limit-4.ll
7 ↗(On Diff #192789)

"@v will be put in sbss, but @r won't be put in sdata"

test/CodeGen/RISCV/sdata-limit-8.ll
7 ↗(On Diff #192789)

"@v will be put in sbss and @r will be put in sdata"

test/CodeGen/RISCV/sdata-local-sym.ll
8 ↗(On Diff #192789)

"@v will b put in sbss and @r will be put in sdata"

This revision is now accepted and ready to land.Apr 10 2019, 10:50 AM
shiva0217 updated this revision to Diff 194626.Apr 10 2019, 6:51 PM
shiva0217 edited the summary of this revision. (Show Details)

Update patch to address Alex's comments.
Hi Alex, thanks for the review.

This revision was automatically updated to reflect the committed changes.