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 ...