This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][GlobalISel] Optimize Combine Funnel Shift
ClosedPublic

Authored by chuongg3 on Aug 10 2023, 2:02 AM.

Diff Detail

Event Timeline

chuongg3 created this revision.Aug 10 2023, 2:02 AM
chuongg3 requested review of this revision.Aug 10 2023, 2:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2023, 2:02 AM

Is it possible to add a vector test to the mir tests too?

llvm/include/llvm/Target/GlobalISel/Combine.td
439

I would move these down to next to the other funnel shift combines

918–921

Can you format this to be a shorter line.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2649

This comment maybe doesn't add much

2657

It would be good to have a comment here that explains what this is doing.

chuongg3 updated this revision to Diff 549395.Aug 11 2023, 7:54 AM
chuongg3 marked 4 inline comments as done.
chuongg3 updated this revision to Diff 549399.Aug 11 2023, 7:59 AM
arsenm added inline comments.Aug 11 2023, 11:04 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
862

Can you try using the new combiner patterns added in 63afb70503bf254bb3cdd8acc6bd1ad25c66e0d8

chuongg3 updated this revision to Diff 549893.Aug 14 2023, 5:47 AM
chuongg3 marked an inline comment as done.

Used new MIR pattern matcher for the combiner

chuongg3 updated this revision to Diff 549894.Aug 14 2023, 5:52 AM
arsenm added inline comments.Aug 15 2023, 4:16 PM
llvm/include/llvm/Target/GlobalISel/Combine.td
876

not sure if ImmLeafs work

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
2640

Don't need to construct an APInt just to do uge

chuongg3 updated this revision to Diff 550741.Aug 16 2023, 7:06 AM
chuongg3 marked 2 inline comments as done.

Removed an unnecessary creation of APInt pointed out by arsenm

chuongg3 updated this revision to Diff 551053.Aug 17 2023, 2:16 AM

Moved funnel-shift.ll test update to D155484

Pierre-vh added inline comments.Aug 22 2023, 12:39 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
862

In theory this should just be

(match (G_FSHR $x, $y, 0))
(apply (COPY $x, $y))

Same deal below but with G_FSHL

funnel_shift_overshift can stay the same though, there's no way to express that using MIR patterns easily right now.

chuongg3 updated this revision to Diff 552324.Aug 22 2023, 5:37 AM
chuongg3 marked an inline comment as done.
chuongg3 added inline comments.Aug 24 2023, 8:10 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
876

It looks like ImmLeaf will currently take a type and use an int64_t for the internal type. These patterns want to handle any types, so it doesn't look like they can be used as-is here.

Are there existing aarch64 IR tests that you can add a global-isel RUN line to? That way we can also check how the codegen compares to SelectionDAG,

llvm/test/CodeGen/AArch64/GlobalISel/combine-fshl.mir
3

No need for -O0 or abort=1

chuongg3 updated this revision to Diff 555702.Sep 4 2023, 3:49 AM
chuongg3 marked an inline comment as done.
aemerson accepted this revision.Sep 4 2023, 6:33 PM
This revision is now accepted and ready to land.Sep 4 2023, 6:33 PM
This revision was automatically updated to reflect the committed changes.