This is an archive of the discontinued LLVM Phabricator instance.

[ARM] i16 insert-of-extract to VINS pattern
ClosedPublic

Authored by dmgreen on Jan 26 2021, 9:56 AM.

Details

Summary

This adds another tablegen fold that converts an i16 odd-lane-insert of an even-lane-extract into a VINS. We extract the existing f32 value from the destination register and VINS the new value into it. The rest of the backend then is able to optimize the INSERT_SUBREG/COPY_TO_REGCLASS/EXTRACT_SUBREG.

Diff Detail

Event Timeline

dmgreen created this revision.Jan 26 2021, 9:56 AM
dmgreen requested review of this revision.Jan 26 2021, 9:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2021, 9:56 AM
simon_tatham added inline comments.Feb 4 2021, 4:56 AM
llvm/lib/Target/ARM/ARMInstrMVE.td
1883

I don't understand this part – if $src1 is already an MQPR, why does it need a COPY_TO_REGCLASS?

dmgreen added inline comments.Feb 4 2021, 11:58 AM
llvm/lib/Target/ARM/ARMInstrMVE.td
1883

Hmm. I added it as a typecast, essentially. Otherwise the INSERT_SUBREG fails to make it through the tablegen type checks. Trying it as (INSERT_SUBREG (v4f32 MQPR:$src1), ... gives an error that looks like the insertsubreg has conflicting input types
(INSERT_SUBREG:{ *:[v4f32] } MQPR:{ *:[] }:$src1

simon_tatham added inline comments.Feb 5 2021, 1:05 AM
llvm/lib/Target/ARM/ARMInstrMVE.td
1883

Ah, I see – it's not that you needed to convert from MQPR to MQPR, it's just that a side effect of that is that you also get to convert from v8i16 to v4f32, which was what you really needed.

In that case, is that implicit conversion from v8i16 to v4f32 acting as a bitcast, or a VECTOR_REG_CAST? Does this pattern do the right thing when tested big-endian?

dmgreen added inline comments.Feb 5 2021, 7:52 AM
llvm/lib/Target/ARM/ARMInstrMVE.td
1883

The COPY machine instruction (which is what a COPY_TO_REGCLASS will eventually turn into) loses any type info so will always act as a VECTOR_REG_CAST. So should be fine in big and little endian - and seems to be OK in my testing.

simon_tatham accepted this revision.Feb 5 2021, 7:55 AM
simon_tatham added inline comments.
llvm/lib/Target/ARM/ARMInstrMVE.td
1883

Fair enough. Thanks for checking.

This revision is now accepted and ready to land.Feb 5 2021, 7:55 AM
This revision was automatically updated to reflect the committed changes.