This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Add a pass to merge all of the constant global arrays into one pool.
ClosedPublic

Authored by stefanp on Jul 19 2023, 10:54 AM.

Details

Summary

On PowerPC the number of TOC entries must be kept low for large
applications. In order to reduce the number of constant global arrays
we can pool them into one structure and then access them as the base
address of that structure plus some offset. The constant global arrays
may be arrays of i8 which are constant strings but they may also be
arrays of i32, i64, etc....

Diff Detail

Event Timeline

stefanp created this revision.Jul 19 2023, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 10:54 AM
stefanp requested review of this revision.Jul 19 2023, 10:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 10:54 AM
stefanp added a reviewer: Restricted Project.Jul 19 2023, 10:56 AM
stefanp updated this revision to Diff 542521.Jul 20 2023, 7:46 AM

Did a clang-format on one file where I had missed the formatting.

amyk added inline comments.Jul 27 2023, 9:44 AM
llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
61

Silly question - Does this need to be an else if? Just wondering since the alignments are an else if so maybe I am missing something.

91

Question: I think we discussed this before when talking about constant merging, but how do we determine which passes to preserve again? And how does this different from the constant pool merge patch?

112

Is there a reason why this is declared outside, rather than inside the class?

280
298

Do we mean user here?

llvm/lib/Target/PowerPC/PPCTargetMachine.cpp
105
amyk added inline comments.Jul 27 2023, 9:44 AM
llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
164

This might be just me and is a minor nit, but I think it may read better of it was something like "Constant data of Global:" or something like that.

This is minor though, so you can feel free to go with whichever one you like.

llvm/test/CodeGen/PowerPC/stringpool.ll
1 ↗(On Diff #542521)

Can we add -ppc-asm-full-reg-names to this?

amyk added a comment.Jul 27 2023, 10:27 AM

Also can we add in the title of the review that this is for strings specifically?

amyk added a comment.Jul 27 2023, 12:47 PM

More group code review comments.

Also, actually, is it more accurate to call this "array pooling" rather than "string pooling"?

llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
50

Do we need to check size and alignment together?

58

Can this be slightly more elaborated upon?

114

Can we get the context later on when we need it?

120

Can we document this function (and why instruction and non-GlobalValue constant users are valid)?

127

Question:

  • Can we elaborate why we can't have GlobalValues here?
  • Can we add a test case where we have a constant using constant?
  • Can we add a test where a constant is a global value and where it is not?
128
  • Can we do CurrentUser here instead of UserConstant since we are casting twice?
  • And can we pull this statement out after after 122-123 since we're reducing nesting?
137

Can we document this function?

145

Is the hasInitializer() check necessary?

Also, is it possible to have a constant that is undef?

148
  • Can we document why the global cannot have a section and metadata?
  • Can we combine these into ||?
llvm/test/CodeGen/PowerPC/stringpool.ll
31 ↗(On Diff #542521)

Can we add a test where we do addis->addi?

amyk added inline comments.Aug 2 2023, 10:16 AM
llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
79

New group code review questions:

  • Have we considered alignment based string pool struct?
  • Have we considered making this target independent and making a Discourse discussion regarding this?
193

Could we update the name of this SmallVector to better indicate it's purpose?

211

Is our element index big enough?
Should it be size_t, or at least a size large enough for what we are targeting? It should support any platform's size_t.

243

Could we please add some documentation to this function?

stefanp updated this revision to Diff 552705.Aug 23 2023, 7:27 AM
stefanp marked 12 inline comments as done.

Addressed most of the comments in the review. There is only one more test case that I want to look at.
Turned the pass on by default on PowerPC.

stefanp added inline comments.Aug 23 2023, 7:27 AM
llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
50

At this point I've added a limitation where where the alignment cannot be larger than the size of the element in order to be included in the pool. This means that at this point it is sufficient to just check the alignment here.

79

I think that both of these are good suggestions.

I think that one struct per alignment may be a good idea but I think it should be controlled by an option and added as a later patch.

This pass does not rely on anything that is PowerPC specific and so it could be turned into a target independent pass. However, I would prefer to try this for PowerPC first because I go ahead and try to make it target independent for a number of targets.

91

When I look at passes to preserve I generally add this pass and then look to see which analysis passes are added after this one in the pipeline. After that I check the passes that are added to see if it makes sense to preserve them. It's not really a hard science and it is a bit of a judgement call because I have to have an understanding of what each pass does.

This pass is in a very different place in the pipeline compared to the constant pool patch. This pass is before instruction selection and so it is an IR pass and not a machine IR pass.

112

Moved inside the class.

114

Moved this out.

148

I'm not going to merge these two into one if statement. They have nothing to do with each other and I think it's more clear if they are separate.

lei added a comment.Aug 23 2023, 9:04 AM

nit: can we rename the new test files tobe mergeable-string-pool.ll mergeable-string-pool-large.ll?

amyk added a comment.Aug 23 2023, 9:43 AM

Group review comments.
Also, can we update the patch description to elaborate on what we mean by "strings"?

llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
103

nit: Maybe renaming this is more clear? MergeableStrings, or something similar?

119

Can we rename this function so it is more clear?

122

(For here and below) Instead of dyn_cast<>, maybe isa<> is better?

133
156
169

Check to see if line 168 can be inlined.

224

Can we add a TODO to split the struct if the offsets get too big?

llvm/test/CodeGen/PowerPC/stringpool.ll
5 ↗(On Diff #552705)

If this is on by default, I think -ppc-merge-string-pool=true can be removed for here and other places.

stefanp retitled this revision from [PowerPC] Add a pass to merge all of the constant globals into one pool. to [PowerPC] Add a pass to merge all of the constant global arrays into one pool..Aug 23 2023, 10:53 AM
stefanp edited the summary of this revision. (Show Details)
stefanp updated this revision to Diff 552851.Aug 23 2023, 12:37 PM

Addressed the remainder of the review comments.
Added a couple of TODOs.
Removed the option from the test cases as it is on by default now.

amyk added a comment.Aug 24 2023, 9:01 AM

More group review comments.

It would be good add an IR to IR test for this patch, as well.

llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
112

Please check that this is where the namespace should end.

222

Can we reword the comment (to describe the intention behind the code and to elaborate on why we're doing it)?

Also, update the TODO below to reflect that we're calling it "pool" rather than "struct".

227

Can we also update the naming for AnonStruct?
Maybe just ConstantPool?

231

Rename this maybe so we know what the global is for?

248–252

This whole part can probably be put into the function that inserts the GEP before the users.

254
  • This function name update might be more clear.
  • Add a comment as to why we need to insert the GEPs.
257
  • Maybe it would be safer to limit the globals only to private linkage (move the check to collectCandidateConstants)?
  • Can this either be either private or internal linkage?
stefanp updated this revision to Diff 553460.Aug 25 2023, 6:56 AM
stefanp marked 4 inline comments as done.

Added better variable naming.
Moved the linkage check to the place where the candidates are collected.
Moved the array copy into the function that adds the GEP.

amyk added a comment.Aug 25 2023, 8:45 AM

Submitting group code review.

llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
95

Can adding the constant GEP in PHI affect SCEV (line 93/94)?

236

Use PooledStructType in this line.

276

Rewording of the function for clarity.

279–280

Can these variables be inlined?

286
288

Maybe just:

289

Instead of G, use the full word (Global) for all G* variables.

297

Expand on this a bit more (that the GEP is added into the PHI itself).

304

Indicate that for regular instructions we add the GEP before hand.

322
  • Add a comment here to describe the situation (adding the GEP inline).
  • Also, move this before line 338 (where it is first used).
326–327
  • Clarify the comment a bit?
  • Can we move all of the casts/checks/asserts (also on 331, 335) on top rather than at the bottom (so we check everything that is valid, then we go about the replacing)?
  • Add an IR test for this case.
llvm/test/CodeGen/PowerPC/O3-pipeline.ll
76 ↗(On Diff #553460)

Can we check if the Dominator Tree Construction is expected, or if it's just there because it's a module pass.

stefanp updated this revision to Diff 553691.Aug 25 2023, 7:26 PM
stefanp marked 6 inline comments as done.

Added another test case that is IR to IR for this pass.
Addressed remaining comments.

llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
112

I've moved this down. None of the functions needs to be accessed from outside of the class so there is no reason to take them outside of the namespace.

llvm/test/CodeGen/PowerPC/O3-pipeline.ll
76 ↗(On Diff #553460)

I did check and this seems to be related to the fact that this is a module pass. I did try to add setPreservesAll and I still got the extra passes so I'm not sure there is much that can be done about this.

lei added a comment.Aug 30 2023, 6:57 AM

For new passes, maybe we can commit it as off first and then subsequent patch to turn it on by default? That way if it causes any issues we can just revert the default on patch.

llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
214

Wondering if maybe we need to do more investigation into this, as in is there any negative benefits to merging strings if they are < N.

223

nit: Do we need the braces for the for-loop?

306–336

Was thinking maybe we can split this nested if into an early exit of the loop.
Basically:

if (!UserInstruction) {
  // User is a constant type.
  ....
  continue;
}

// User is a valid instruction.

if (PHINode *UserPHI = dyn_cast<PHINode>(UserInstruction)) {
  // User is a PHI node instruction.
  .....
  continue;

}

// The user is a valid instruction that is not a PHINode.
314

nit: indent

stefanp marked 4 inline comments as done.Sep 6 2023, 10:38 AM

For new passes, maybe we can commit it as off first and then subsequent patch to turn it on by default? That way if it causes any issues we can just revert the default on patch.

I'm going to split this patch as you suggested. This patch will have the pass off by default and then I will add another quick patch to github where it is turned on.

llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
214

I've added an option that we can use to do this investigation in the future.

stefanp updated this revision to Diff 556062.Sep 6 2023, 11:37 AM

Addressed a couple of review comments. Split this patch into two.
This patch now has the pass off by default. A second patch will turn it on by default.

lei accepted this revision as: lei.Sep 6 2023, 12:11 PM

LGTM
Thx

llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp
318

nit: extra empty line

This revision is now accepted and ready to land.Sep 6 2023, 12:11 PM
amyk accepted this revision as: amyk.Sep 7 2023, 6:32 AM

Also LGTM and thanks for answering the questions and addressing comments.

stefanp updated this revision to Diff 556155.Sep 7 2023, 8:14 AM

Rebased to top of main. Fixed last nit.

This revision was landed with ongoing or failed builds.Sep 7 2023, 8:15 AM
This revision was automatically updated to reflect the committed changes.