Page MenuHomePhabricator

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

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



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)


Why is this condition relevant?


nit: s/equalt/equal/


nit: indentation

sdesmalen added inline comments.Jul 21 2021, 8:39 AM

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

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.


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.

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

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