This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Enable safe for 32bit vins* P10 instructions
ClosedPublic

Authored by ZarkoCA on Apr 27 2021, 10:56 AM.

Details

Summary

Correctly emit vinsinstructions that are safe in 32bit mode.

Diff Detail

Unit TestsFailed

Event Timeline

ZarkoCA created this revision.Apr 27 2021, 10:56 AM
ZarkoCA requested review of this revision.Apr 27 2021, 10:56 AM
jsji added a reviewer: Restricted Project.Apr 27 2021, 12:10 PM
nemanjai added inline comments.Apr 28 2021, 3:59 AM
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?

ZarkoCA added inline comments.Apr 28 2021, 6:52 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
10344

It looks like all of the pattern matches for VINS* in PPCInstrPrefix.td hardcode i64:
eg:

  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.

nemanjai added inline comments.Apr 28 2021, 8:32 AM
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.

lebedev.ri retitled this revision from Disable vinsw, vinsd, and vins[wd][lr]x P10 instructions in P10 to [PowerPC] Disable vinsw, vinsd, and vins[wd][lr]x P10 instructions in P10.Apr 28 2021, 8:34 AM
ZarkoCA updated this revision to Diff 343455.May 6 2021, 10:26 AM
  • Enable safe for 32bit vins p10 instructions
ZarkoCA retitled this revision from [PowerPC] Disable vinsw, vinsd, and vins[wd][lr]x P10 instructions in P10 to [PowerPC] Enable safe for 32bit vins* P10 instructions .May 6 2021, 10:27 AM
ZarkoCA edited the summary of this revision. (Show Details)
ZarkoCA marked 2 inline comments as done.May 6 2021, 10:30 AM
ZarkoCA added inline comments.
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.

nemanjai added inline comments.May 7 2021, 6:32 AM
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).

ZarkoCA marked an inline comment as done.May 7 2021, 8:03 AM
ZarkoCA added inline comments.
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.

ZarkoCA updated this revision to Diff 343684.May 7 2021, 8:04 AM
  • Removing unnecessary isPPC64 check
  • fix formatting
nemanjai accepted this revision.May 10 2021, 4:20 AM

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.

This revision is now accepted and ready to land.May 10 2021, 4:20 AM
This revision was automatically updated to reflect the committed changes.