This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Support more types for ZEXT/SEXT with Global Isel
ClosedPublic

Authored by chuongg3 on Aug 4 2023, 8:45 AM.

Diff Detail

Event Timeline

chuongg3 created this revision.Aug 4 2023, 8:45 AM
chuongg3 requested review of this revision.Aug 4 2023, 8:45 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 8:45 AM
arsenm added inline comments.Aug 7 2023, 2:25 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
1046–1070 ↗(On Diff #547226)

Is it possible to separate this from the lowerEXT parts?

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
5951

This shouldn't need to consider the legality of intermediate ops and fail, just unconditionally do this and let the new artifacts legalize

chuongg3 updated this revision to Diff 549353.Aug 11 2023, 5:39 AM
chuongg3 marked 2 inline comments as done.

Split combiner into a seperate commit.
Unconditionally lowers Extends, independent of the intermediate instruction

dmgreen added inline comments.Aug 15 2023, 7:57 AM
llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
5952

Can reduce the indenting now.

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

Can you add a comment here explaining that this is trying to convert larger extends into two smaller ones.

525

(Query.Types[0].isVector()) -> Query.Types[0].isVector()

llvm/test/CodeGen/AArch64/GlobalISel/legalize-build-vector.mir
144–145

Is this with update_mir_test_checks? Maybe just replace %ext with %3 in the code below.

chuongg3 updated this revision to Diff 550351.Aug 15 2023, 8:49 AM
chuongg3 marked 4 inline comments as done.
aemerson accepted this revision.Aug 15 2023, 1:24 PM

A few comments but otherwise LGTM, thanks for the new tests.

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
5951–5952

No need for MidRegister, this can be simplified to:

auto NewExt = MIRBuilder.buildInstr(MI.getOpcode(), {MidTy}, {Src});
...
auto UnmergeSrc = MIRBuilder.buildUnmerge(EltTy, NewExt);
...`
5958–5966

Same here, it's less verbose to just create the LLTs and pass those to buildInstr() instead of passing Registers.

// ZExt the vectors
Register ZExtResTy = DstTy.changeElementCount(DstTy.getElementCount().divideCoefficientBy(2));
auto ZExtRes1 = MIRBuilder.buildInstr(MI.getOpcode(), {ZExtResTy}, {UnmergeSrc.getReg(0)});
auto ZExtRes2 = MIRBuilder.buildInstr(MI.getOpcode(), {ZExtResTy}, {UnmergeSrc.getReg(1)});
This revision is now accepted and ready to land.Aug 15 2023, 1:24 PM
chuongg3 updated this revision to Diff 550738.Aug 16 2023, 6:58 AM
chuongg3 marked 2 inline comments as done.

Rectified comments made by aemerson
Instructions are built using register types instead of creating a register for the instruction to be built

chuongg3 updated this revision to Diff 550758.Aug 16 2023, 8:07 AM

Formatted the patch