Page MenuHomePhabricator

[Aarch64][SVE] Add DAG combine rules for gather loads and sext/zext
ClosedPublic

Authored by andwar on Nov 28 2019, 4:05 AM.

Details

Summary

These changes allow us to support sign-extending gather loads with the existing intrinsics (i.e. @llvm.aarch64.sve.ld1.gather.*).

Diff Detail

Event Timeline

andwar created this revision.Nov 28 2019, 4:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2019, 4:05 AM
sdesmalen added inline comments.Dec 2 2019, 5:53 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9991

AArch64Subtarget *Subtarget isn't used by performSVEAndCombine, so you can remove this change.

12159

The condition Src.hasOneUse() is not in the SVEAndCombine, so you probably want to add one there as well.
Also please make sure both cases have a unit test.

andwar updated this revision to Diff 232297.Dec 5 2019, 3:39 AM
andwar marked an inline comment as done.
  • Apply suggestions from @sdesmalen (e.g. add !Src.hasOneUse() in performANDCombine)
  • Removed a bunch of setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::nxv2i64, Legal), which are not needed for this patch
  • Simplified performSignExtendInRegCombine
  • Added patterns for sext_inreg (required for the new tests vvvv)
  • Added tests that verify that the new DAG Combine rules are not used when the result of gather load has multiple uses
andwar updated this revision to Diff 232365.Dec 5 2019, 9:06 AM

Remove a typo after rebase (uimm5s2 vs uim5s4)

sdesmalen accepted this revision.Dec 10 2019, 5:53 AM

Aside from two nits, LGTM!

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
9957

nit: Are these asserts necessary?

9971

nit: Can you make this a switch statement with a default that returns SDValue()?

This revision is now accepted and ready to land.Dec 10 2019, 5:53 AM
This revision was automatically updated to reflect the committed changes.