This patch removes the uneccessary mf/mtvsr generated in conjunction
with xscvdpsxws/xscvdpuxws.
Details
Details
- Reviewers
nemanjai saghir kamaub amyk - Group Reviewers
Restricted Project - Commits
- rG4195ed995993: [PowerPC] Improved codegen related to xscvdpsxws/xscvdpuxws
Diff Detail
Diff Detail
Unit Tests
Unit Tests
Event Timeline
Comment Actions
Removed unintended change, moved pattern to more appropriate location, reduced added complexity to 600 as it still works.
Comment Actions
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 | ||
|---|---|---|
| 8 | 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. | |
| llvm/lib/Target/PowerPC/PPCInstrVSX.td | ||
|---|---|---|
| 2815 | This should be XSCVDPUXWS? | |
| llvm/test/CodeGen/PowerPC/test-vector-insert.ll | ||
| 2 | nit: Move this comment under the RUN lines. | |
| 3 | I'm not sure why P8 is LE run only line, and P7 is BE run line only. 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? | |
| llvm/test/CodeGen/PowerPC/test-vector-insert.ll | ||
|---|---|---|
| 8 | good point, thanks! | |
Comment Actions
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 | ||
|---|---|---|
| 1 | 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. | |
| 5 | Code generation on Power9 is important as well. Please add that test. | |
| 15 | nit: s/Codgen/Codegen | |
Comment Actions
This modifies a test case introduced in this commit: https://github.com/llvm/llvm-project/commit/3678df5ae6618eec656ae0ea0dab3be09d73bc9a
Comment Actions
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? | |
| 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! | |
Comment Actions
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 should be XSCVDPUXWS?