This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVEIntrinsicOpts] Optimize tbl+dup into dup+extractelement
ClosedPublic

Authored by junparser on Mar 26 2021, 4:11 AM.

Details

Summary

According to D99324, this patch try to convert pattern sve_tbl(vec, sve_dup_x(idx)) to sve_dup_x(extractelement(vec, idx)) which simplify lowering and isel in later phase. Try to solve the problem for both ACLE and auto-vectorisation.

TestPlan: check-llvm

Diff Detail

Unit TestsFailed

Event Timeline

junparser created this revision.Mar 26 2021, 4:11 AM
junparser requested review of this revision.Mar 26 2021, 4:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2021, 4:11 AM
paulwalker-arm added inline comments.Mar 26 2021, 4:31 AM
llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp
466–467

Can you use Builder.CreateVectorSplat here?

469–470

I'm never really sure what to do in these instances but do we need to worry about the original DupXIntrI? Given we know its operand is constant it's easily removed if this was the only use.

junparser updated this revision to Diff 333755.Mar 28 2021, 7:07 PM

Address the comment.

junparser added inline comments.Mar 28 2021, 7:12 PM
llvm/lib/Target/AArch64/SVEIntrinsicOpts.cpp
466–467

done

469–470

replaceAllUsesWith and eraseFromParent usually appear together. we can ignore the Dup intrinsic here. However, we should remove it since it has no side effect, no use. Remove it can be safe.

Hi, thank you for the patch! The optimisation seems sound to me -- just a few comments on the test side. 😄

llvm/test/CodeGen/AArch64/sve-tbl-dupx.ll
2

I don't think -dce is necessary here.

3

Worth adding:

; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
...
; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
; WARN-NOT: warning

To make sure your 2>%t is used. This block can usually be found in SVE tests. 😄

89

A few of these are unused at the minute. You can remove:

llvm.aarch64.sve.dup.x.nxv8f16
llvm.aarch64.sve.dup.x.nxv2f32
llvm.aarch64.sve.dup.x.nxv4f32
llvm.aarch64.sve.dup.x.nxv2f64
junparser updated this revision to Diff 333787.Mar 29 2021, 2:12 AM

Address comments

Hi, thank you for the patch! The optimisation seems sound to me -- just a few comments on the test side. 😄

Thanks for review!

joechrisellis accepted this revision.Mar 29 2021, 3:53 AM

LGTM. 😄

This revision is now accepted and ready to land.Mar 29 2021, 3:53 AM
paulwalker-arm added inline comments.Mar 29 2021, 4:20 AM
llvm/test/CodeGen/AArch64/sve-tbl-dupx.ll
3

Not a problem for this patch but for information once D98856 lands this practice will no longer be needed as the key warning will error by default for opt/llc tests.

13

This shows what I meant by my previous comment in that all the tests check for this stray instruction and so it seem preferable to me for SVEIntrinsicOpts to remove it when it's knowingly no longer used.

junparser updated this revision to Diff 333821.Mar 29 2021, 4:56 AM
junparser added inline comments.
llvm/test/CodeGen/AArch64/sve-tbl-dupx.ll
13

done, thanks for remind.

paulwalker-arm accepted this revision.Mar 29 2021, 5:08 AM