This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Global Isel Funnel Shift Lowering
ClosedPublic

Authored by chuongg3 on Jul 17 2023, 9:22 AM.

Details

Summary

Global Isel:
Does not lower FSHR if the amount of shifts is a constant.
Lowers FSHL to FSHR instead of EXTR.

Diff Detail

Event Timeline

chuongg3 created this revision.Jul 17 2023, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 9:22 AM
chuongg3 requested review of this revision.Jul 17 2023, 9:22 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 9:22 AM
chuongg3 updated this revision to Diff 541087.Jul 17 2023, 9:25 AM
chuongg3 retitled this revision from [AArch64] SelectionDAG Funnel Shift Lowering to [AArch64] Funnel Shift Lowering.
chuongg3 edited the summary of this revision. (Show Details)

GlobalISel:
Does not lower FSHR if the amount of shifts is a constant.
Lowers FSHL to FSHR instead of EXTR.

chuongg3 retitled this revision from [AArch64] Funnel Shift Lowering to [AArch64] Global Isel Funnel Shift Lowering.Jul 18 2023, 1:21 AM
chuongg3 edited the summary of this revision. (Show Details)
dmgreen added inline comments.Jul 20 2023, 7:05 AM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
901–903

This an be removed now.

1044

Can you format the patch. Also this doesn't need the (..) brackets, and single line if's can drop the {..} brackets in llvm.

dmgreen added inline comments.Jul 20 2023, 7:06 AM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
905

I think that the s64, s32 combo will never come up. The s32, x64 only comes up due to the way this is being legalized.

You lower at least s37 and s128 without any attempts to legalize them. There may be some opportunities, at least for s37.

chuongg3 updated this revision to Diff 542803.Jul 21 2023, 1:49 AM
chuongg3 marked 3 inline comments as done.

You lower at least s37 and s128 without any attempts to legalize them. There may be some opportunities, at least for s37.

I would expect that i128 is more important than i37. It may be a job for combining, as opposed to legalization (or better generic code for legalizing fsh's)

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
67

number of shifts -> shift amount

79

It might be worth making this an APInt, so that it doesn't need to convert again below.

85

These have more () brackets than they need.

Allen added a subscriber: Allen.Jul 25 2023, 6:42 AM
chuongg3 updated this revision to Diff 545978.Aug 1 2023, 2:55 AM
chuongg3 marked 3 inline comments as done.

Amount is created as an APInt instead of converting it later on
Replaced number of shifts in comments with shift amount

arsenm added a subscriber: arsenm.Aug 1 2023, 6:53 AM
arsenm added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
1051–1053

Can just construct without the =

1055–1083

Could this just be an optimization combine instead? You're ultimately just falling back to the default legalization

dmgreen added inline comments.Aug 1 2023, 8:22 AM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
1055–1083

Can you explain what you mean? My understanding was that returning true meant "this has been legalized", so the G_FSHR with a constant shift between 0 and BitWidth is legal. (So it ends up reusing the tablegen patterns for fshr). Non-constant shifts are illegal (expanded) and fshl are transformed to a fshr.

llvm/test/CodeGen/AArch64/funnel-shift.ll
476

This one may be incorrect I think. It should be returning the other operand from the test below (ideally without a extr).

chuongg3 updated this revision to Diff 547705.Aug 7 2023, 3:57 AM
chuongg3 marked 2 inline comments as done.

Funnel Shift with 0 shift amount are turned into MOV instructions

dmgreen added inline comments.
llvm/include/llvm/Target/GlobalISel/Combine.td
387 ↗(On Diff #547705)

Can you split this out into a separate patch (and move it down next to funnel_shift_to_rotate, maybe with a name that includes funnel_shift). It could do with a mir test to show the combine works in isolation. It's probably best to keep the optimization separate from the legalization though.

chuongg3 updated this revision to Diff 548926.Aug 10 2023, 1:59 AM
chuongg3 marked 3 inline comments as done.

Split Combine and Legalize into two patches

dmgreen added inline comments.Aug 11 2023, 12:25 AM
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
1055–1057

This looks like it is already checked above

1063–1065

This can drop the { } brackets from single statement if blocks.

1067–1068

This can be removed too now.

chuongg3 updated this revision to Diff 549397.Aug 11 2023, 7:55 AM
chuongg3 marked 3 inline comments as done.

Can you add the llvm/test/CodeGen/AArch64/funnel-shift.ll test changes back to this patch too? Otherwise this looks good to me.

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
1050

Maybe:

// Lower non-constant shifts and leave zero shifts to the optimizer.
chuongg3 updated this revision to Diff 549833.Aug 14 2023, 2:19 AM
chuongg3 marked an inline comment as done.
chuongg3 updated this revision to Diff 551052.Aug 17 2023, 2:14 AM

Updated funnel-shift.ll in this patch instead of in D157591

dmgreen accepted this revision.Aug 17 2023, 8:06 AM

Thanks. This LGTM

This revision is now accepted and ready to land.Aug 17 2023, 8:06 AM
This revision was landed with ongoing or failed builds.Aug 22 2023, 2:33 AM
This revision was automatically updated to reflect the committed changes.