This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Fix incorrect DAG combines when extracting fixed-width from scalable vectors
ClosedPublic

Authored by david-arm on Sep 28 2021, 6:38 AM.

Details

Summary

We were previously silently generating incorrect code when extracting a
fixed-width vector from a scalable vector. This is worse than crashing,
since the user will have no indication that this is currently unsupported
behaviour. I have fixed the code to only perform DAG combines when safe
to do so, i.e. the input and output vectors are both fixed-width or
both scalable.

Test added here:

CodeGen/AArch64/sve-extract-scalable-vector.ll

Diff Detail

Event Timeline

david-arm created this revision.Sep 28 2021, 6:38 AM
david-arm requested review of this revision.Sep 28 2021, 6:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2021, 6:38 AM
david-arm added inline comments.Sep 28 2021, 6:39 AM
llvm/test/CodeGen/AArch64/sve-extract-scalable-vector.ll
6 ↗(On Diff #375554)

Hi all, my apologies. I just realised I've uploaded the wrong test. I will fix asap!

david-arm updated this revision to Diff 375556.Sep 28 2021, 6:42 AM
  • Actually upload the correct test. :)
Matt added a subscriber: Matt.Sep 28 2021, 12:08 PM
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
20605

Can we replace :

if (NVT.isScalableVector() == ConcatSrcVT.isScalableVector() && ConcatSrcNumElts == ExtNumElts)

by:

if (NVT.getElementCount() == ConcatSrcVT.getElementCount() && NVT.isScalableVector())
david-arm updated this revision to Diff 377129.Oct 5 2021, 2:48 AM

Hi @CarolineConcatto, thanks for the suggestion! I couldn't add the && NVT.isScalableVector() bit because I also want that particular DAG combine to trigger for fixed-width vectors too.

peterwaller-arm accepted this revision.Oct 5 2021, 8:16 AM
This revision is now accepted and ready to land.Oct 5 2021, 8:16 AM