Page MenuHomePhabricator

[AArch64][SVE] Fix crash with icmp+select
ClosedPublic

Authored by CarolineConcatto on Apr 14 2021, 8:06 AM.

Details

Summary

This patch changes the lowering of SELECT_CC from Legal to Expand for scalable
vector and adds support for scalable vectors in performSelectCombine.

When selecting the nodes to lower in visitSELECT it checks if it is possible to
use SELECT_CC in cases where SETCC is followed by SELECT. visistSELECT checks
if SELECT_CC is legal or custom to replace SELECT by SELECT_CC.
SELECT_CC used to be legal for scalable vector, so the node changes to
SELECT_CC. This used to crash the compiler as there is no support for SELECT_CC
with scalable vectors. So now the compiler lowers to VSELECT instead of
SELECT_CC.

Diff Detail

Event Timeline

CarolineConcatto requested review of this revision.Apr 14 2021, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 8:06 AM

Can you also add support for predicates?

This currently also fails:

define <vscale x 16 x i1> @icmp_select_nxv16i1(<vscale x 16 x i1> %a, <vscale x 16 x i1> %b, i64 %x0) {
  %mask = icmp eq i64 %x0, 0
  %sel = select i1 %mask, <vscale x 16 x i1> %a, <vscale x 16 x i1> %b
  ret <vscale x 16 x i1> %sel
}
llvm/test/CodeGen/AArch64/select-sve.ll
550 ↗(On Diff #337458)

I don't think testing these wider types is necessary, as they will just rely on splitting the select, for which we already have tests.

Addressing reviewer's comment

  • add support for missing types: nxv2i1, nxv4i1, nxv8i1, nxv16i1
  • remove redundant tests
CarolineConcatto marked an inline comment as done.Thu, Apr 15, 3:14 AM

Thank you @sdesmalen for pointing out that I missed some types.
Now nxv[2|4|8|16]i1 works. I also removed the redundant tests that you've asked for.

Thanks for the changes @CarolineConcatto

llvm/test/CodeGen/AArch64/select-sve.ll
222 ↗(On Diff #337682)

You can also remove the unpacked integer tests (e.g. <vscale x [2|4|8] x i8>, <vscale x [2|4] x i16>, <vscale x 2 x i32>`) as those types are promoted to a legal type.

c-rhodes added inline comments.Thu, Apr 15, 3:45 AM
llvm/test/CodeGen/AArch64/select-sve.ll
135 ↗(On Diff #337682)

Just an observation, I noticed we have 2 tests for SVE selects:

  • llvm/test/CodeGen/AArch64/sve-select.ll
  • llvm/test/CodeGen/AArch64/select-sve.ll

Can these be combined?

  • Rebase with main
  • Remove Expand for MVT::nxv8i8, MVT::nxv4i16, MVT::nxv2i32, as it is done by default
  • Remove tests for MVT::nxv8i8, MVT::nxv4i16, MVT::nxv2i32

Thank you @sdesmalen and @c-rhodes for the review.
I'll comment you reviews in here, because the file select-sve.ll is deleted and the tests moved to sve-select.ll
@sdesmalen I have removed the tests you have asked.
I also remove the Expand in the Lowering for these types:
{MVT::nxv8i8, MVT::nxv4i16, MVT::nxv2i32}
as I believe this happens by default.
@c-rhodes the tests from select-sve.ll are now in sve-select.ll and select-sve.ll deleted.
That is already on main.

Matt added a subscriber: Matt.Sat, Apr 17, 10:27 AM
This revision is now accepted and ready to land.Mon, Apr 19, 1:12 AM
efriedma added inline comments.Mon, Apr 19, 10:49 AM
llvm/test/CodeGen/AArch64/sve-select.ll
255

Not really related to this patch, but maybe looking into why we aren't combining the cset/sbfx into csinv.

This revision was automatically updated to reflect the committed changes.
llvm/test/CodeGen/AArch64/sve-select.ll
255

Thank you @efriedma.
We will have a look and create a patch for this in the future.
But like you wrote it is not related to this patch.