Page MenuHomePhabricator

[AArch64] Redundant masks in downcast long multiply
ClosedPublic

Authored by NickGuy on Oct 22 2020, 6:25 AM.

Details

Summary

Adds patterns to catch masks preceeding a long multiply,
and generating a single umull/smull instruction instead.

Diff Detail

Event Timeline

NickGuy created this revision.Oct 22 2020, 6:25 AM
NickGuy requested review of this revision.Oct 22 2020, 6:25 AM

Looks nice to me. Is it worth adding mul (sext_inreg, sext i32) patterns too in case one operand is sext and the other is being masked? mul is commutative so I think it would only be two extra patterns, one for sext and one for zext.

llvm/lib/Target/AArch64/AArch64InstrInfo.td
1479

Nit: Can you line up the ('s for the input and the output
The other patterns below have output type's too. It's probably worth adding those just for consistency.

NickGuy updated this revision to Diff 300659.Oct 26 2020, 7:08 AM

Looks nice to me. Is it worth adding mul (sext_inreg, sext i32) patterns too in case one operand is sext and the other is being masked? mul is commutative so I think it would only be two extra patterns, one for sext and one for zext.

Added, though I did have some difficulties hand-crafting something that would use these patterns (hence no specific tests)

llvm/lib/Target/AArch64/AArch64InstrInfo.td
1479

Parentheses aligned and output put types added, thanks :)

Something like https://godbolt.org/z/q376ob maybe?

llvm/lib/Target/AArch64/AArch64InstrInfo.td
1480

GPR32 I think, for the sext input. And then the output can use the GPR32 directly. Like the SMADDLrrr pattern below.

NickGuy updated this revision to Diff 302229.Mon, Nov 2, 2:39 AM
NickGuy updated this revision to Diff 302277.Mon, Nov 2, 7:32 AM
dmgreen added inline comments.Mon, Nov 2, 7:43 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
1483

This pattern isn't needed I don't think. The other four you have here look good to me, but I would remove the whitespace and maybe reorder them.

llvm/test/CodeGen/AArch64/aarch64-mull-masks.ll
25

Can you add a commuted test too, where the and and the conv are used the other way around. Same for smull. They should match automatically I think, with the patterns that are already here.

NickGuy updated this revision to Diff 302291.Mon, Nov 2, 8:30 AM

This pattern isn't needed I don't think

Removed

but I would remove the whitespace and maybe reorder them.

Done

Can you add a commuted test too

Added, the patterns do indeed match

dmgreen accepted this revision.Tue, Nov 3, 1:39 AM

Brilliant, thanks. LGTM

This revision is now accepted and ready to land.Tue, Nov 3, 1:39 AM
This revision was landed with ongoing or failed builds.Tue, Nov 3, 2:12 AM
This revision was automatically updated to reflect the committed changes.