Correctly emit vinsinstructions that are safe in 32bit mode.
Details
- Reviewers
nemanjai bmahjour amyk stefanp power-llvm-team - Group Reviewers
Restricted Project - Commits
- rG0c41f77857fc: [PowerPC] Enable safe for 32bit vins* P10 instructions
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
90 ms | x64 debian > LLVM.Verifier::token1.ll |
Event Timeline
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
10344 | It would seem that all we need to change this condition and the one below to not emit PPCISD::VECINSERT for 64-bit element widths (v2i64, v2f64). Why do we need to disable this lowering on 32-bit targets altogether? |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
10344 | It looks like all of the pattern matches for VINS* in PPCInstrPrefix.td hardcode i64: def : Pat<(v16i8 (PPCvecinsertelt v16i8:$vDi, i32:$rA, i64:$rB)), (VINSBLX $vDi, InsertEltShift.Sub32Left0, $rA)>; ... foreach i = [0, 1] in def : Pat<(v2i64 (PPCvecinsertelt v2i64:$vDi, i64:$rA, (i64 i))), (VINSD $vDi, !mul(i, 8), $rA)>; } So we can't emit the VECINSERT safely in 32bit mode due to this. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
10344 | Sure, so those won't match. You might be able to change i64 to iPTR (I'm not sure about that) or provide patterns with i32 instead of i64. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
10344 | Thanks for the suggestion and help. It's much better to emit these when we can in 32bit mode. | |
llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
2758 ↗ | (On Diff #343455) | I preferred to split the 32/64bit implementations mainly to keep 64bit as is. I noticed that there were no other predicate definitions in this file and they can be moved if that's preferred. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
1238 | I don't really understand how we are custom lowering this on 32-bit targets now since you've added this. Where are the PPC-specific insert nodes coming from? | |
llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
2758 ↗ | (On Diff #343455) | This is fine. |
2771 ↗ | (On Diff #343455) | Nit: line too long (here and elsewhere). |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
1238 | We don't need this now, I don't think. Forgot to remove it in the previous diff. | |
llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
2758 ↗ | (On Diff #343455) | Thanks. |
2771 ↗ | (On Diff #343455) | I noticed that but also there are several lines in IsISA3_1, HasVSX, IsLittleEndian and IsISA3_1, HasVSX, IsBigEndian, IsPPC64 that were too long as well. Thought it may be ok but I fixed it now for this case. |
LGTM other than the nit that can be addressed on the commit.
llvm/lib/Target/PowerPC/PPCInstrPrefix.td | ||
---|---|---|
2769–2770 ↗ | (On Diff #343684) | The operand should be lined up with the first operand of the node it belongs to: def : Pat<(v4f32 (PPCvecinsertelt v4f32:$vDi, (f32 (load iaddr:$rA)), i32:$rB)), Similarly on other similar lines. |
2771 ↗ | (On Diff #343455) | We haven't done super well with keeping the lines in target description files to 80 lines, but we should still try to do so on new code. |
I don't really understand how we are custom lowering this on 32-bit targets now since you've added this. Where are the PPC-specific insert nodes coming from?