Page MenuHomePhabricator

[PPC] Check for PPC64 when emitting 64bit specific VSX nodes when pattern matching built vectors
ClosedPublic

Authored by ZarkoCA on Dec 7 2020, 2:38 PM.

Details

Summary

Some of the pattern matching in PPCInstrVSX.td and node lowering involving vectors assumes 64bit mode. This patch disables some of the unsafe pattern matching and lowering of BUILD_VECTOR in 32bit mode.

Diff Detail

Unit TestsFailed

TimeTest
70 msx64 windows > LLVM.CodeGen/XCore::threads.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\llc.exe -march=xcore < C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\CodeGen\XCore\threads.ll

Event Timeline

ZarkoCA created this revision.Dec 7 2020, 2:38 PM
ZarkoCA requested review of this revision.Dec 7 2020, 2:38 PM
Whitney added a subscriber: Whitney.Dec 9 2020, 8:21 PM
Xiangling_L added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
9389–9390

minor: Is that better to adjust the comment accordingly like:
Under 64-bit mode, BUILD_VECTOR nodes that ...

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

I am wondering why do we leave alone [HasVSX, IsBigEndian] and [HasVSX, HasOnlySwappingMemOps, IsBigEndian] and HasVSX, HasP8Vector, NoP9Vector, IsBigEndian]?

2427–2428

minor: This line should follow #2425?

llvm/test/CodeGen/PowerPC/ppc-32bit-build-vector.ll
4

It seems -mcpu=pwr8 already implies HasAltivec and HasVSX enabled, so -mattr=+altivec and -mattr=+vsx are not necessary.

68

Suggestion: The testcase can be further simplified into the following by removing tedious zext instruction and shortening the numbers of vector.

define dso_local fastcc void @BuildVectorICE() unnamed_addr #0 {
  entry:
    br label %while.body
    while.body:                                       ; preds = %while.body, %entry
    %newelement = phi i32 [ 0, %entry ], [ %5, %while.body ]
    %0 = insertelement <4 x i32> <i32 undef, i32 0, i32 0, i32 0>, i32 %newelement, i32 0
    %1 = load <4 x i32>, <4 x i32>* undef, align 1
    %2 = add <4 x i32> %1, %0
    %3 = shufflevector <4 x i32> %2, <4 x i32> undef, <4 x i32> <i32 1, i32 undef, i32 undef, i32 undef>
    %4 = add <4 x i32> %2, %3
    %5 = extractelement <4 x i32> %4, i32 0
    br label %while.body
}
ZarkoCA marked 4 inline comments as done.Dec 11 2020, 6:11 AM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCInstrVSX.td
2412

That's a good question, I tried disabling these so as to have minimal disruption to existing code. And did it by elimination.

So, as far as I can see the potentially big problem in the pattern matching is later implementations like [HasVSX, IsISA3_0, HasDirectMove, IsBigEndian] were implemented without considering 32bit codegen and I added a check there and moved on to earlier ones. At some point, I started hitting test case failures and found that codegen in those cases was correct. And in those cases I found that there were checks added for 32bit like in this patch: https://reviews.llvm.org/D17711.

In short, I tried to do the safe thing for 32bit VSX without major disruption to existing test cases and codegen unless I could plainly tell it wasn't correct. Which in those cases I wasn't able to.

llvm/test/CodeGen/PowerPC/ppc-32bit-build-vector.ll
68

Thanks a lot! That simplifies it and I'm still able to get the compiler error without the patch.

ZarkoCA updated this revision to Diff 311207.Dec 11 2020, 6:13 AM
ZarkoCA marked an inline comment as done.

Addressed comments:

  • simplified and modified test case per suggestion
  • added comment
Xiangling_L added inline comments.Dec 11 2020, 7:55 AM
llvm/lib/Target/PowerPC/PPCInstrVSX.td
2412

I see your point here. I am asking because I tried adding IsPPC64 for [HasVSX, HasP8Vector, NoP9Vector, IsBigEndian] and found no regressions. So I feel the current situation is kinda messy by having partial fixes and disablement for 32bit mode. But I understand that you want this patch to be conservative correct.

I am wondering Is that possible for us to add IsPPC64 for all predicates where no existing testcases would be failed? But if it would take you too much time, I can live with that for now. Since as we discussed offline that we do have plan to verify them one by one for 32bit mode in the near future.

llvm/test/CodeGen/PowerPC/ppc-32bit-build-vector.ll
65

Sorry that I forgot to mention, attributes attributes #0 are also not necessary.

ZarkoCA added inline comments.Dec 11 2020, 8:04 AM
llvm/lib/Target/PowerPC/PPCInstrVSX.td
2412

I see your point here. I am asking because I tried adding IsPPC64 for [HasVSX, HasP8Vector, NoP9Vector, IsBigEndian] and found no regressions. So I feel the current situation is kinda messy by having partial fixes and disablement for 32bit mode. But I understand that you want this patch to be conservative correct.

Ah ok, I likely missed this instance. I will have a look and update the patch. Thanks.

And yes, the plan is to enable 32Bit VSX over time either by extending the implementation here if possible.

ZarkoCA updated this revision to Diff 311233.Dec 11 2020, 8:33 AM

Added further IsPPC64 checks for Big Endian VSX
removed function attributes from test case.

ZarkoCA marked 3 inline comments as done.Dec 11 2020, 8:34 AM
Xiangling_L accepted this revision.Dec 11 2020, 11:08 AM

LGTM. Thanks!

This revision is now accepted and ready to land.Dec 11 2020, 11:08 AM
This revision was landed with ongoing or failed builds.Dec 12 2020, 12:28 PM
This revision was automatically updated to reflect the committed changes.