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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
| Time | Test | |
|---|---|---|
| 50 ms | x64 windows > LLVM.CodeGen/XCore::threads.ll |
Event Timeline
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 9389–9390 | minor: Is that better to adjust the comment accordingly like: | |
| 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
} | |
| 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. | |
| 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 | ||
| 64 | Sorry that I forgot to mention, attributes attributes #0 are also not necessary. | |
| llvm/lib/Target/PowerPC/PPCInstrVSX.td | ||
|---|---|---|
| 2412 |
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. | |
Added further IsPPC64 checks for Big Endian VSX
removed function attributes from test case.
minor: Is that better to adjust the comment accordingly like:
Under 64-bit mode, BUILD_VECTOR nodes that ...