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....
Details
- Reviewers
nemanjai lei amyk hubert.reinterpretcast sfertile - Group Reviewers
Restricted Project - Commits
- rG84e2fd7ee4a9: [PowerPC] Add a pass to merge all of the constant global arrays into one pool.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
60,050 ms | x64 debian > MLIR.Examples/standalone::test.toy |
Event Timeline
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 |
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? |
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:
| |
128 |
| |
137 | Can we document this function? | |
145 | Is the hasInitializer() check necessary? Also, is it possible to have a constant that is undef? | |
148 |
| |
llvm/test/CodeGen/PowerPC/stringpool.ll | ||
31 ↗ | (On Diff #542521) | Can we add a test where we do addis->addi? |
llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp | ||
---|---|---|
79 | New group code review questions:
| |
193 | Could we update the name of this SmallVector to better indicate it's purpose? | |
211 | Is our element index big enough? | |
243 | Could we please add some documentation to this function? |
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.
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. |
nit: can we rename the new test files tobe mergeable-string-pool.ll mergeable-string-pool-large.ll?
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. |
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.
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? | |
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 |
| |
257 |
|
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.
Submitting group code review.
llvm/lib/Target/PowerPC/PPCMergeStringPool.cpp | ||
---|---|---|
94 | Can adding the constant GEP in PHI affect SCEV (line 93/94)? | |
235 | Use PooledStructType in this line. | |
275 | Rewording of the function for clarity. | |
278–279 | Can these variables be inlined? | |
285 | ||
287 | Maybe just: | |
288 | Instead of G, use the full word (Global) for all G* variables. | |
296 | Expand on this a bit more (that the GEP is added into the PHI itself). | |
303 | Indicate that for regular instructions we add the GEP before hand. | |
321 |
| |
325–326 |
| |
llvm/test/CodeGen/PowerPC/O3-pipeline.ll | ||
76 | Can we check if the Dominator Tree Construction is expected, or if it's just there because it's a module pass. |
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 | 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. |
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. 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 |
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. |
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.
Do we need to check size and alignment together?