Implemented the option to omit Power10 instructions from save stubs via the option --no-power10-stubs or --power10-stubs=no on lld. --power10-stubs= will override the other option. --power10-stubs=auto also exists to use the default behaviour (ie allow Power10 instructions in stubs).
Details
- Reviewers
stefanp nemanjai MaskRay - Group Reviewers
Restricted Project - Commits
- rG36192790d84b: [PowerPC][PC Rel] Implement option to omit Power10 instructions from stubs
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
A few general comments.
lld/ELF/Config.h | ||
---|---|---|
74 | We have a "yes", but does it need to be here, too? | |
lld/ELF/Driver.cpp | ||
764 | Add documentation for the function. | |
777 | Do we need to handle power10_stubs? | |
lld/ELF/Options.td | ||
453 | nit: maybe use the full word instructions here and in the following lines. | |
455 | Describe the default behaviour? | |
lld/ELF/Thunks.cpp | ||
925 | Unrelated change? | |
930–952 | Can we maybe try to have a more descriptive name than inst2? (here and in the other places) | |
lld/test/ELF/ppc64-toc-call-to-pcrel.s | ||
29 | No need for the extra # |
Addressed comments
lld/ELF/Config.h | ||
---|---|---|
74 | After a bit of discussion, since we don't have a concrete implementation for the "yes" option yet, the consensus is to keep it out for now. I'll remove it from the help message as well. | |
lld/ELF/Driver.cpp | ||
777 | That option is handled by the variable NoP10; I'll add a comment to make that more clear. | |
lld/ELF/Thunks.cpp | ||
925 | This change was made so that the commented instruction would better line up with the new instructions added for better readablity. |
I have a few comments. Most of them are nits but there is a functional issue as well.
For the testing:
Do we have a test for the PPC64R2SaveStub in the situation where the offset fits in 34 bits?
lld/ELF/Driver.cpp | ||
---|---|---|
769 | nit: if (!args.hasArg(OPT_power10_stubs_eq) && args.hasArg(OPT_no_power10_stubs)) return P10Stub::No; Mainly because you only use NoP10 in one place at this point. | |
lld/ELF/Options.td | ||
448 | nit: | |
lld/ELF/Thunks.cpp | ||
937 | This sequence does not match with what you are using as an offset. Since you have a valid r2 and are computing the offset from the TOC pointer you should be adding to that. | |
940 | Same here as above. | |
977 | nit: | |
983 | You need to have the off-8 in both cases. The way that this is done is quite confusing beause in one case you subtract 8 in the lambda and in the other you just subtract here inline. I would say just subtract in the computation of off and then you don't have to do it twice in two different places. Secondly, instead of casting to uint16_t you are better off just using a mask (off & 0xffff). | |
1014 | nit: | |
1058 | I see that these lambdas are used in a lot of places to do the same thing. It might be better to have a static function instead of defining the same lambda three times. | |
1060 | nit: Same as above. Just add compute the offset with the 8 difference here. | |
lld/test/ELF/ppc64-pcrel-call-to-toc.s | ||
18 | nit: | |
llvm/include/llvm/Object/ELF.h | ||
95 | Are these two instruction masks used? |
Removed lambda functions and made a static function, included a R2SaveStub test case that covers if it fits in 34 bits with option --no-power10-stubs.
There is one more thing that does not look quite right. It may be an encoding problem for one of the instructions. I found it in the test but you will have to trace it back to the place where it is generated in order to fix it.
lld/ELF/Driver.cpp | ||
---|---|---|
771 | You may even be able to inline this since it is only used in one place that I can see. | |
lld/ELF/Options.td | ||
453 | nit: | |
lld/ELF/Thunks.cpp | ||
935 | nit: | |
lld/test/ELF/ppc64-pcrel-call-to-toc.s | ||
55 | This is not correct. addis 12, 11, -1 The encoding above may not be correct. |
Added a note on one of the functions
lld/ELF/Driver.cpp | ||
---|---|---|
766 | For this function here, I realize we can inline all the ifs into a giant return statement - is there any opinions on this? I thought the if statements might make this a bit more readable, but if it is preferred that there is only 1 return statement I can make that change as well. |
Just nits this time around.
lld/ELF/Driver.cpp | ||
---|---|---|
766 | Personally I'm happy with it like this. I'm not a fan of huge if statements but I could be persuaded otherwise depending on what other reviewers have to say... | |
lld/ELF/Options.td | ||
454 | nit: | |
lld/ELF/Thunks.cpp | ||
941 | nit: | |
lld/test/ELF/ppc64-pcrel-call-to-extern.s | ||
110 | nit: | |
135 | nit: | |
160 | nit: 0x10030168 - 0x10030010 = 0x158 0x150 = 336 I'm not going to comment on all of these. Please take a look at the comments where numbers have changed and make sure that they are correct. |
I just have one final nit otherwise LGTM. Feel free to fix that on commit.
Please wait a couple of days and see if @MaskRay has any further comments.
lld/test/ELF/ppc64-pcrel-call-to-extern.s | ||
---|---|---|
135 | nit: 0x10030160 - 0x10020010 = 0x10150 not 0x10160 I think it should be: 0x10030170 |
Thanks for answering the questions I previously had on this patch.
lld/ELF/Driver.cpp | ||
---|---|---|
764 | nit: remove the space after = and before flags. |
The used code sequence has 2 bugs so the output will crash. I fixed them in 7198c87f42f6c15d76b127c1c63530e9b4d5dd39 and added my notes to https://maskray.me/blog/2023-02-26-linker-notes-on-power-isa
We have a "yes", but does it need to be here, too?