This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SVE] Lower MULHU/MULHS nodes to umulh/smulh instructions
ClosedPublic

Authored by bsmith on Apr 14 2021, 8:28 AM.

Details

Summary

Mark MULHS/MULHU nodes as legal for both scalable and fixed SVE types,
and lower them to the appropriate SVE instructions.

Additionally now that the MULH nodes are legal, integer divides can be
expanded into a more performant code sequence.

Diff Detail

Event Timeline

bsmith created this revision.Apr 14 2021, 8:28 AM
bsmith requested review of this revision.Apr 14 2021, 8:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2021, 8:28 AM
paulwalker-arm added inline comments.Apr 15 2021, 7:58 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.h
92–93

As the general rule we use the common node name suffixed with _PRED, so in this case these should be MULHS_PRED and MULHU_PRED.

llvm/lib/Target/AArch64/AArch64SVEInstrInfo.td
192

Just a note, because of my other comment, to say that this is fine, here we typically prefer the AArch64 names as you've used here, although it'll be nicer still if you maintained the existing alphabetical ordering :)

llvm/test/CodeGen/AArch64/sve-fixed-length-int-mulh.ll
6

Should this be VBITS_EQ_2048?

68

Given you're using EQ_256 I doubt the #min logic means anything. That said, why not VBITS_GE_256, shouldn't the code be the same for larger vectors?

394

As there's no NEON instruction for i64 based vectors I'm wondering if it's worth using SVE for this case as well? much like we do for ISD::MUL.

bsmith updated this revision to Diff 338074.Apr 16 2021, 5:56 AM
bsmith marked 5 inline comments as done.
bsmith edited the summary of this revision. (Show Details)
  • Rename ISD nodes to match common names
  • Preserve some alphabetical ordering
  • Fix 2048 vector width tests
  • Update fixed width tests to test vector widths smaller than the given value
  • Allow generation of SVE mulh instructions for neon sized i64 vectors
  • Add test to check divide expansion that can now happen
  • Add minsize attribute to some SVE divide tests to prevent divide expansion from happening
Matt added a subscriber: Matt.Apr 17 2021, 9:15 AM
paulwalker-arm accepted this revision.Apr 19 2021, 4:06 AM
paulwalker-arm added inline comments.
llvm/test/CodeGen/AArch64/sve-fixed-length-int-mulh.ll
3–6

Please add RUN lines for all the support vector lengths.

Also, just in case somebody wonders why, can you add a comment saying there's no validation for splitting vector operations because the necessary MULH DAG combine does no apply to illegally typed operations.

292

Weirdly SVE is not being used here. Is the output different when SVE is disable?

780

Same comment as smulh_v4i32.

This revision is now accepted and ready to land.Apr 19 2021, 4:06 AM
bsmith added inline comments.Apr 20 2021, 5:42 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-int-mulh.ll
3–6

There already is a comment as such, on line 12 (23 in the new patch)

292

No, it's the same without SVE enabled. NEON has patterns to match 128-bit mulh nodes (but not 64-bit as above), as it can use the smull2+smull pattern below. Perhaps we should still fall back to SVE instead of this sequence? (Or I just fix the comment..)

paulwalker-arm added inline comments.Apr 20 2021, 5:59 AM
llvm/test/CodeGen/AArch64/sve-fixed-length-int-mulh.ll
3–6

That must have been where "my" idea came from :)

292

Thanks. In which case fixing the comment works for me.