Page MenuHomePhabricator

[SVE][AArch64] Improve code generation for vector_splice for Imm > 0
ClosedPublic

Authored by CarolineConcatto on Jul 19 2021, 6:40 AM.

Details

Summary

This patch implements vector_splice in tablegen for all cases when the
Immediate is positive and lower than the known minimum value of
a scalable vector.
Vector_splice can be implemented using SVE instruction EXT.
For instance :

@llvm.experimental.vector.splice(Vector_1, Vector_2, Imm)
@llvm.experimental.vector.splice(<A,B,C,D>, <E,F,G,H>, 1) ==> <B, C, D, E>
    EXT  Vector_1, Vector_2, Imm              // Vector_1 = B, C, D + Vector_2 = E

Depends on D105633

Diff Detail

Event Timeline

CarolineConcatto requested review of this revision.Jul 19 2021, 6:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2021, 6:40 AM

Is this patch still work-in-progress, or is it ready to review? (it looks like the latter)

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7432

Why is this condition relevant?

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
2402

nit: s/equalt/equal/

2410

nit: indentation

sdesmalen added inline comments.Jul 21 2021, 8:39 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
7432

Just realised the why; it's because of the immediate range for EXT.

nit: the ranges overlap, so you can write:

auto Idx = Op.getConstantOperandAPInt(2);
if (Idx >= -1 && Idx < Ty.getVectorMinNumElements())
  return Op;
return SDValue;
CarolineConcatto marked 4 inline comments as done.
CarolineConcatto retitled this revision from [WIP] Improve code generation for vector_splice for Imm > 0 to [SVE][AArch64] Improve code generation for vector_splice for Imm > 0.
  • Remove WIP from the title.
  • Simplify test in the lowering function.
  • Fix indentation in tablegen
david-arm added inline comments.Jul 23 2021, 8:53 AM
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
245

Just a thought, but do we even need a Min here since it always seems to be 0 anyway? We could just compare directly against 0 in the if statement below.

llvm/test/CodeGen/AArch64/named-vector-shuffles-sve.ll
206

Hi @CarolineConcatto it's great that you're adding these unpacked vector test cases! Would it be possible to test a case like this as well?

%res = call <vscale x 4 x half> @llvm.experimental.vector.splice.nxv4f16(<vscale x 4 x half> %a, <vscale x 4 x half> %b, i32 2)

The reason I'm interested is because the splice intrinsic maps to the ext instruction only for packed vector types. I'm not entirely sure if the index immediate passed to ext will still be correct in this case as it may need scaling.

  • Add test for unpacked types with idx != 0
  • Remove Min parameter from SelectEXTImm
CarolineConcatto marked 2 inline comments as done.Jul 26 2021, 1:19 AM
CarolineConcatto added inline comments.
llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp
245

Yes, it is possible.
I followed the same pattern as in` bool SelectCntImm(SDValue N, SDValue &Imm) `.
But I believe there is no problem with changing it. We could add the minimum parameter again if this function is used in the future with other minimum ranges.

david-arm accepted this revision.Jul 26 2021, 1:21 AM

LGTM! Thanks for making the changes. :)

This revision is now accepted and ready to land.Jul 26 2021, 1:21 AM
This revision was landed with ongoing or failed builds.Jul 26 2021, 3:46 AM
This revision was automatically updated to reflect the committed changes.
CarolineConcatto marked an inline comment as done.
sdesmalen added inline comments.Jul 26 2021, 4:12 AM
llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
2411

The 'AddedComplexity' isn't needed for these patterns.