Page MenuHomePhabricator

[SelectionDAG] Fixed the scalable vectors issue on WidenVecRes_OverflowOp&WidenVecRes_SELECT
Needs ReviewPublic

Authored by tangxingxin1008 on Sep 23 2021, 4:11 AM.

Details

Summary
This change make WidenVecRes_OverflowOp&WidenVecRes_SELECT work
for scalable vectors.

This patch base on https://reviews.llvm.org/D110058

Signed-off-by: Eric Tang <tangxingxin1008@gmail.com>

Diff Detail

Event Timeline

tangxingxin1008 requested review of this revision.Sep 23 2021, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 23 2021, 4:11 AM

I know they're both small changes but I think it would be good to split this patch up into separate parts: overflow and select. They're conceptually distinct changes. Then we could have more targeted tests for each operation and each change.

Also RISC-V would benefit from the select change, e.g., adding this sort of test to test/CodeGen/RISCV/rvv/vselect-int-rv32.ll and vselect-int-rv64.ll

define <vscale x 3 x i8> @vmerge_vv_nxv3i8(<vscale x 3 x i8> %va, <vscale x 3 x i8> %vb, <vscale x 3 x i1> %cond) {
  %vc = select <vscale x 3 x i1> %cond, <vscale x 3 x i8> %va, <vscale x 3 x i8> %vb
  ret <vscale x 3 x i8> %vc
}
llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
854–855

This looks suspicious to me. What's it for?

I know they're both small changes but I think it would be good to split this patch up into separate parts: overflow and select. They're conceptually distinct changes. Then we could have more targeted tests for each operation and each change.

Also RISC-V would benefit from the select change, e.g., adding this sort of test to test/CodeGen/RISCV/rvv/vselect-int-rv32.ll and vselect-int-rv64.ll

define <vscale x 3 x i8> @vmerge_vv_nxv3i8(<vscale x 3 x i8> %va, <vscale x 3 x i8> %vb, <vscale x 3 x i1> %cond) {
  %vc = select <vscale x 3 x i1> %cond, <vscale x 3 x i8> %va, <vscale x 3 x i8> %vb
  ret <vscale x 3 x i8> %vc
}

thanks, I will be changed according to your advice.

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
854–855

In AArch sve, only [nxv2i1, nxv4i1, nxv8i1, nxv16i1] [nxv16i8, nxv8i16, nxv4i32, nxv2i64] are legal, so <nxv1i8, nxv1i1> would be widened to <nxv16i8, nxv16i1> by WidenVecRes_OverflowOp.
In below code:

unsigned OtherNo = 1 - ResNo;
EVT OtherVT = N->getValueType(OtherNo);
if (getTypeAction(OtherVT) == TargetLowering::TypeWidenVector) {
  SetWidenedVector(SDValue(N, OtherNo), SDValue(WideNode, OtherNo));
}

getTypeAction(OtherVT) would convert <nxv1i1> to <nxv2i1>.

So the SDValue(N, OtherNo) value type is <nxv2i1>, but SDValue(WideNode, OtherNo) is <nxv16i1>. In this case, SetWidenedVector will be failed in

assert(Result.getValueType() ==
           TLI.getTypeToTransformTo(*DAG.getContext(), Op.getValueType()) &&
           "Invalid type for widened vector");

In my understand, I do this change like as SetSplitVector does. is that right?

For SetWidenedVector, this change reduced the restrictions. In AArch64 sve, normally, SetWidenedVector only support nxv1i1 -> nxv2i1, after this change, the nxv1i1 -> nxv4i1, nxv1i1 -> nxv8i1, nxv1i1 -> nxv16i1 can also work.

david-arm resigned from this revision.Sep 30 2022, 2:42 AM

Nothing seems to have happened on this patch for a year.

Herald added a project: Restricted Project. · View Herald TranscriptSep 30 2022, 2:42 AM