Page MenuHomePhabricator

[AArch64] Generate vector signed/unsigned mul and mla/mls long
ClosedPublic

Authored by bmakam on Oct 2 2014, 2:38 PM.

Details

Summary

Dear Tim, Chad, Ana, Jiangning and Hao,

This is my first patch to the LLVM project.

We are seeing missed opportunities to generate widened mul/mla/mls in AArch64
ARM had an implementation that was ported to AArch64.

Diff Detail

Event Timeline

bmakam updated this revision to Diff 14348.Oct 2 2014, 2:38 PM
bmakam retitled this revision from to [AArch64] Generate vector signed/unsigned mul and mla/mls long.
bmakam updated this object.
bmakam edited the test plan for this revision. (Show Details)
bmakam added a subscriber: Unknown Object (MLST).
t.p.northover edited edge metadata.Oct 2 2014, 3:07 PM

Hi Balaram,

Thanks for working on this. I think the code is mostly OK (which is a relief, otherwise ARM would already be broken). I've got a few small suggestions though:

lib/Target/AArch64/AArch64ISelLowering.cpp
1738

Function names should start with a lower case letter (this isn't the only example).

1845

It's not called VMULL on AArch64.

1853

Ditto

1856–1857

Could you comment on which CPUs this is known to be beneficial for.

test/CodeGen/AArch64/aarch64-smull.ll
288

CHECK-LABEL. It's also a good idea to include the label's ':' in the check line, otherwise you can match weird directives at the end of the function like

.size	umull_extvec_v2i32_v2i64, .Ltmp1-umull_extvec_v2i32_v2i64
bmakam updated this revision to Diff 14379.Oct 3 2014, 7:20 AM
bmakam edited edge metadata.

updated patch addressing Tim's comments.

mcrosier added inline comments.Oct 3 2014, 7:26 AM
test/CodeGen/AArch64/aarch64-smull.ll
228

Please be consistent with your CHECK directives. Specifically, either do or don't include a space between the ; and the CHECK for each test. Personally, I prefer a space and I believe this is the general style within the community.

bmakam updated this revision to Diff 14382.Oct 3 2014, 8:49 AM

Updated patch after addressing both Chad's and Tim's comments.

bmakam updated this revision to Diff 14391.Oct 3 2014, 11:43 AM

Actually, including the label's ':' in the check line in this commit. Previously, I only changed CHECK with CHECK-LABEL

Thanks,
Balaram

bmakam updated this revision to Diff 14411.Oct 3 2014, 3:07 PM
t.p.northover accepted this revision.Oct 6 2014, 9:44 AM
t.p.northover edited edge metadata.

This looks OK to me now. Thanks for updating it.

Tim.

This revision is now accepted and ready to land.Oct 6 2014, 9:44 AM
mcrosier edited edge metadata.Oct 7 2014, 7:49 PM

Committed in r219276.

Tim,
Would you like for me to add some logic so this is only enabled for A53/A57 or are you find with this being enabled for Cyclone as well?

Chad

mcrosier closed this revision.Oct 7 2014, 7:50 PM
bmakam added a comment.Oct 7 2014, 7:58 PM

Thanks Tim and Chad :)

Congrats and welcome to the club, Balaram.