This is an archive of the discontinued LLVM Phabricator instance.

[DAG][AArch64][SVE] Fix VLS mulh code generation
ClosedPublic

Authored by DavidTruby on Jan 6 2022, 5:10 AM.

Details

Summary

Introducing lowering for extending loads regressed an optimisation in
VLS codegen. Since the extend is merged into the load before the mulh is
generated, a standard mul followed by a lsr and truncating store is used
instead. This patch causes the mulh to be generated before the extends
are merged into the loads.

Diff Detail

Unit TestsFailed

Event Timeline

DavidTruby created this revision.Jan 6 2022, 5:10 AM
DavidTruby requested review of this revision.Jan 6 2022, 5:10 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2022, 5:10 AM
peterwaller-arm accepted this revision.Jan 6 2022, 8:47 AM
This revision is now accepted and ready to land.Jan 6 2022, 8:47 AM

Whilst not wrong this looks like something that should be canonicalised earlier. Is it possible to add this logic to SelectionDAG::getVectorShuffle() and utilise SelectionDAG::getConstant()? I ask because there are several functions of isConstOrConstSplat's ilk and we don't really want to have to add similar support to all of them.

Matt added a subscriber: Matt.Jan 7 2022, 7:30 AM

Whilst not wrong this looks like something that should be canonicalised earlier. Is it possible to add this logic to SelectionDAG::getVectorShuffle() and utilise SelectionDAG::getConstant()? I ask because there are several functions of isConstOrConstSplat's ilk and we don't really want to have to add similar support to all of them.

I'm not 100% sure what you mean here so I thought I ought to check; do you mean that SelectionDAG::getVectorShuffle() should return a BUILD_VECTOR generated using SelectionDAG::getConstant() in cases like this where the vector shuffle is a constant splat?

paulwalker-arm added a comment.EditedJan 10 2022, 4:17 AM

Yes, that was what I was thinking, because it seemed odd that such a constant representation exists by this point in the pipeline. However, I can see that it's coming from the test itself and so I've changed my mind and am now wondering if there actually a problem that needs solving here?

The tests are using a non-standard immediate representation (aka "fixed vector shuffle of an inserted constant") that LLVM would have converted to a constant vector (aka <i16 8, i16 8,....) long before it gets to the code generator. This is backed up by running the test through clang, which results in smul when expected.

This makes me think that to protect the MULS/MULU functionality, you just need to update the test itself to remove the non-standard representation.

DavidTruby closed this revision.Jan 14 2022, 6:26 AM

I changed the tests as suggested above in rG0af1808f9b99 and this issue does go away as suggested so I am closing this revision.