This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Improved codegen related to xscvdpsxws/xscvdpuxws
ClosedPublic

Authored by Conanap on Sep 16 2021, 11:11 AM.

Details

Summary

This patch removes the uneccessary mf/mtvsr generated in conjunction
with xscvdpsxws/xscvdpuxws.

Diff Detail

Event Timeline

Conanap created this revision.Sep 16 2021, 11:11 AM
Conanap requested review of this revision.Sep 16 2021, 11:11 AM
Conanap updated this revision to Diff 373009.Sep 16 2021, 11:16 AM

Removed unintended change, moved pattern to more appropriate location, reduced added complexity to 600 as it still works.

Conanap edited the summary of this revision. (Show Details)Sep 16 2021, 11:49 AM
Conanap updated this revision to Diff 373045.Sep 16 2021, 1:05 PM
Conanap edited the summary of this revision. (Show Details)

Removed complexity and restored a test case

kamaub added a subscriber: kamaub.

This patch seems almost ready to land to me, I'm just a bit concerned about the testing coverage,
is the little endian testing case suppose to target pwr7 as the big endian test does? The default
FileCheck line seems redundant to me, did I misunderstand?

llvm/test/CodeGen/PowerPC/test-vector-insert.ll
9

It seems like this run line is redundant, it produces the same assembly as the big endian specific line above. Maybe the -mcpu=pwr7 can be moved to the first Little-endian specific run line? That line currently only test the target cpu of the test machine.

Conanap updated this revision to Diff 374059.Sep 21 2021, 3:53 PM
Conanap marked an inline comment as done.

Removed AIX test line as it has the same code gen as BE

amyk added a subscriber: amyk.Sep 23 2021, 7:42 AM
amyk added inline comments.
llvm/lib/Target/PowerPC/PPCInstrVSX.td
2815

This should be XSCVDPUXWS?

llvm/test/CodeGen/PowerPC/test-vector-insert.ll
3

nit: Move this comment under the RUN lines.

4

I'm not sure why P8 is LE run only line, and P7 is BE run line only.
Maybe we should have LE/BE run lines for both P7 and P8 for more coverage.
Furthermore, if both the LE/BE checks end up the same, we can do CHECK-P7 and CHECK-P8.

Also, since this looks like it's a Linux test, please add -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr.

llvm/test/CodeGen/PowerPC/vec_conv_fp64_to_i32_elts.ll
16–17

This is an unsigned test case, so should be xscvdpuxws, right?

Conanap updated this revision to Diff 374578.Sep 23 2021, 9:06 AM

Updated test cases, fixed a typo

Conanap marked 3 inline comments as done.Sep 23 2021, 9:06 AM
Conanap added inline comments.
llvm/test/CodeGen/PowerPC/test-vector-insert.ll
9

good point, thanks!

Conanap updated this revision to Diff 374614.Sep 23 2021, 10:33 AM

Added P7 and P8 run lines for BE

amyk accepted this revision as: amyk.Sep 23 2021, 10:43 AM

LGTM. Thanks for addressing comments.

This revision is now accepted and ready to land.Sep 23 2021, 10:43 AM
nemanjai requested changes to this revision.Sep 23 2021, 4:44 PM

There isn't enough information in the test to determine whether this adequately improves code generation (in fact, whether it improves code generation at all).

llvm/test/CodeGen/PowerPC/test-vector-insert.ll
2

Test cases where we improve code generation should be pre-committed as an NFC change so that the review shows just the differences in code generation. It is not easy to evaluate whether the patch improves the code or not without seeing how it changed.

6

Code generation on Power9 is important as well. Please add that test.

16

nit: s/Codgen/Codegen

This revision now requires changes to proceed.Sep 23 2021, 4:44 PM
nemanjai requested changes to this revision.Sep 28 2021, 5:37 AM

I cannot tell without context whether the codegen changes for the f32->i32 convert + insert case (for example @test2 in test/CodeGen/PowerPC/test-vector-insert.ll) but it should change.

llvm/lib/Target/PowerPC/PPCInstrVSX.td
4145

I assume this is the big endian Power9 block (and the below is the little endian). However, I can't confirm that since the context for the patch has disappeared somehow.

4150

What am I missing here? It appears to me that this pattern is exactly the same as the one above it. The same appears to be the case for all of these. Also, why the change in naming convention?

This revision now requires changes to proceed.Sep 28 2021, 5:37 AM
Conanap updated this revision to Diff 375667.Sep 28 2021, 12:37 PM

Added context, changed var names in aptterns

Conanap marked 2 inline comments as done.Sep 28 2021, 12:37 PM
Conanap added inline comments.
llvm/lib/Target/PowerPC/PPCInstrVSX.td
4150

This one is for the unsigned version, the above is for the signed version. Originally, using DblToInt.A gave me problems as $A are now used in two places, so I've changed the other variable names instead. I'll use DblToInt.B instead to match the convention, thanks!

nemanjai requested changes to this revision.Sep 28 2021, 4:53 PM

I really don't understand what happened now. It seems that you have simply reverted to an older version of this patch. The test case appears to not have been pre-committed any longer, the Power9 patterns and test cases are gone. What has happened here?

This revision now requires changes to proceed.Sep 28 2021, 4:53 PM
Conanap updated this revision to Diff 375780.Sep 28 2021, 9:54 PM
Conanap marked an inline comment as done.

Updated correct version of the patch

nemanjai accepted this revision.Sep 30 2021, 3:19 AM

LGTM.

This revision is now accepted and ready to land.Sep 30 2021, 3:19 AM
amyk added a comment.Sep 30 2021, 6:32 AM

Additional nits regarding comments.

llvm/test/CodeGen/PowerPC/test-vector-insert.ll
20
21
This revision was landed with ongoing or failed builds.Sep 30 2021, 12:31 PM
This revision was automatically updated to reflect the committed changes.