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?