This is an archive of the discontinued LLVM Phabricator instance.

Differential D89956

[AArch64] Redundant masks in downcast long multiply
ClosedPublic

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

Details

Reviewers
SjoerdMeijer
dmgreen
t.p.northover
efriedma
Commits
rG54d8627852a6: [AArch64] Redundant masks in downcast long multiply
Summary

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

Diff Detail

Unit TestsFailed

TimeTest
360 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp

Event Timeline

NickGuy created this revision.Oct 22 2020, 6:25 AM
Herald added subscribers: danielkiss, hiraditya, kristof.beyls. · View Herald TranscriptOct 22 2020, 6:25 AM
NickGuy requested review of this revision.Oct 22 2020, 6:25 AM
Harbormaster completed remote builds in B76028: Diff 299943.Oct 22 2020, 7:19 AM
dmgreen added a comment.Oct 22 2020, 7:20 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
1477

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
1477

Parentheses aligned and output put types added, thanks :)

Harbormaster completed remote builds in B76393: Diff 300659.Oct 26 2020, 7:16 AM
dmgreen added a comment.Oct 26 2020, 8:32 AM

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

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

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.Nov 2 2020, 2:39 AM
Harbormaster completed remote builds in B77229: Diff 302229.Nov 2 2020, 2:50 AM
NickGuy updated this revision to Diff 302277.Nov 2 2020, 7:32 AM
dmgreen added inline comments.Nov 2 2020, 7:43 AM
llvm/lib/Target/AArch64/AArch64InstrInfo.td
1481

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.

Harbormaster completed remote builds in B77256: Diff 302277.Nov 2 2020, 8:12 AM
NickGuy updated this revision to Diff 302291.Nov 2 2020, 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

Harbormaster completed remote builds in B77268: Diff 302291.Nov 2 2020, 9:10 AM
dmgreen accepted this revision.Nov 3 2020, 1:39 AM

Brilliant, thanks. LGTM

This revision is now accepted and ready to land.Nov 3 2020, 1:39 AM
This revision was landed with ongoing or failed builds.Nov 3 2020, 2:12 AM
Closed by commit rG54d8627852a6: [AArch64] Redundant masks in downcast long multiply (authored by NickGuy). · Explain Why
This revision was automatically updated to reflect the committed changes.
NickGuy added a commit: rG54d8627852a6: [AArch64] Redundant masks in downcast long multiply.

Revision Contents

PathSize
llvm/
lib/
Target/
AArch64/
5 lines
test/
CodeGen/
AArch64/
28 lines

Diff 299943

llvm/lib/Target/AArch64/AArch64InstrInfo.td

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 1,467 Lines • ▼ Show 20 Lines
} // AddedComplexity = 5 } // AddedComplexity = 5
let AddedComplexity = 5 in { let AddedComplexity = 5 in {
def SMADDLrrr : WideMulAccum<0, 0b001, "smaddl", add, sext>; def SMADDLrrr : WideMulAccum<0, 0b001, "smaddl", add, sext>;
def SMSUBLrrr : WideMulAccum<1, 0b001, "smsubl", sub, sext>; def SMSUBLrrr : WideMulAccum<1, 0b001, "smsubl", sub, sext>;
def UMADDLrrr : WideMulAccum<0, 0b101, "umaddl", add, zext>; def UMADDLrrr : WideMulAccum<0, 0b101, "umaddl", add, zext>;
def UMSUBLrrr : WideMulAccum<1, 0b101, "umsubl", sub, zext>; def UMSUBLrrr : WideMulAccum<1, 0b101, "umsubl", sub, zext>;
def : Pat<(mul (sext_inreg GPR64:$Rn, i32), (sext_inreg GPR64:$Rm, i32)),
(SMADDLrrr (EXTRACT_SUBREG $Rn, sub_32), (EXTRACT_SUBREG $Rm, sub_32), XZR)>;
dmgreenUnsubmitted
Not Done
ReplyInline Actions

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.

dmgreen: Nit: Can you line up the ('s for the input and the output The other patterns below have output…
NickGuyAuthorUnsubmitted
Done
ReplyInline Actions

Parentheses aligned and output put types added, thanks :)

NickGuy: Parentheses aligned and output put types added, thanks :)
def : Pat<(mul (and GPR64:$Rn, 0xFFFFFFFF), (and GPR64:$Rm, 0xFFFFFFFF)),
dmgreenUnsubmitted
Not Done
ReplyInline Actions

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

dmgreen: GPR32 I think, for the sext input. And then the output can use the GPR32 directly. Like the…
(UMADDLrrr (EXTRACT_SUBREG $Rn, sub_32), (EXTRACT_SUBREG $Rm, sub_32), XZR)>;
def : Pat<(i64 (mul (sext GPR32:$Rn), (sext GPR32:$Rm))), def : Pat<(i64 (mul (sext GPR32:$Rn), (sext GPR32:$Rm))),
dmgreenUnsubmitted
Not Done
ReplyInline Actions

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.

dmgreen: This pattern isn't needed I don't think. The other four you have here look good to me, but I…
(SMADDLrrr GPR32:$Rn, GPR32:$Rm, XZR)>; (SMADDLrrr GPR32:$Rn, GPR32:$Rm, XZR)>;
def : Pat<(i64 (mul (zext GPR32:$Rn), (zext GPR32:$Rm))), def : Pat<(i64 (mul (zext GPR32:$Rn), (zext GPR32:$Rm))),
(UMADDLrrr GPR32:$Rn, GPR32:$Rm, XZR)>; (UMADDLrrr GPR32:$Rn, GPR32:$Rm, XZR)>;
def : Pat<(i64 (ineg (mul (sext GPR32:$Rn), (sext GPR32:$Rm)))), def : Pat<(i64 (ineg (mul (sext GPR32:$Rn), (sext GPR32:$Rm)))),
(SMSUBLrrr GPR32:$Rn, GPR32:$Rm, XZR)>; (SMSUBLrrr GPR32:$Rn, GPR32:$Rm, XZR)>;
def : Pat<(i64 (ineg (mul (zext GPR32:$Rn), (zext GPR32:$Rm)))), def : Pat<(i64 (ineg (mul (zext GPR32:$Rn), (zext GPR32:$Rm)))),
(UMSUBLrrr GPR32:$Rn, GPR32:$Rm, XZR)>; (UMSUBLrrr GPR32:$Rn, GPR32:$Rm, XZR)>;
▲ Show 20 Lines • Show All 6,220 Lines • Show Last 20 Lines

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

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc -mtriple=aarch64-none-linux-gnu < %s -o -| FileCheck %s
define i64 @umull(i64 %x0, i64 %x1) {
; CHECK-LABEL: umull:
; CHECK: // %bb.0: // %entry
; CHECK-NEXT: umull x0, w1, w0
; CHECK-NEXT: ret
entry:
%and = and i64 %x0, 4294967295
%and1 = and i64 %x1, 4294967295
%mul = mul nuw i64 %and1, %and
ret i64 %mul
}
define i64 @smull(i64 %x0, i64 %x1) {
; CHECK-LABEL: smull:
; CHECK: // %bb.0: // %entry
; CHECK-NEXT: smull x0, w1, w0
; CHECK-NEXT: ret
entry:
%sext = shl i64 %x0, 32
%conv1 = ashr exact i64 %sext, 32
%sext4 = shl i64 %x1, 32
%conv3 = ashr exact i64 %sext4, 32
dmgreenUnsubmitted
Not Done
ReplyInline Actions

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.

dmgreen: Can you add a commuted test too, where the `and` and the `conv` are used the other way around.
%mul = mul nsw i64 %conv3, %conv1
ret i64 %mul
}