This is an archive of the discontinued LLVM Phabricator instance.

Differential D99219

[AArch64][GlobalISel] Mark G_SBFX/G_UBFX as legal for s32 and s64
ClosedPublic

Authored by paquette on Mar 23 2021, 3:32 PM.

Details

Reviewers
aemerson
Commits
rG1818dc394f31: [AArch64][GlobalISel] Mark G_SBFX/G_UBFX as legal for s32 and s64
Summary

This isn't perfect, since we should also verify that these only use constants...

Diff Detail

Unit TestsFailed

TimeTest
330 msx64 debian > lld.MachO::dependency-info.s

Event Timeline

paquette created this revision.Mar 23 2021, 3:32 PM
Herald added subscribers: danielkiss, hiraditya, kristof.beyls, rovka. · View Herald TranscriptMar 23 2021, 3:32 PM
paquette requested review of this revision.Mar 23 2021, 3:32 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 3:32 PM
aemerson added a comment.Mar 23 2021, 4:05 PM

Legalizer test?

paquette added a child revision: D99224: [AArch64][GlobalISel] Select G_SBFX and G_UBFX.Mar 23 2021, 5:05 PM
paquette updated this revision to Diff 332829.Mar 23 2021, 5:08 PM

Add legalizer tests

Harbormaster completed remote builds in B95372: Diff 332829.Mar 23 2021, 5:13 PM
Harbormaster completed remote builds in B95355: Diff 332805.Mar 23 2021, 8:04 PM
aemerson accepted this revision.Mar 23 2021, 9:50 PM
This revision is now accepted and ready to land.Mar 23 2021, 9:50 PM
This revision was landed with ongoing or failed builds.Mar 24 2021, 11:10 AM
Closed by commit rG1818dc394f31: [AArch64][GlobalISel] Mark G_SBFX/G_UBFX as legal for s32 and s64 (authored by paquette). · Explain Why
This revision was automatically updated to reflect the committed changes.
paquette added a commit: rG1818dc394f31: [AArch64][GlobalISel] Mark G_SBFX/G_UBFX as legal for s32 and s64.
arsenm added a subscriber: arsenm.Mar 24 2021, 11:11 AM
arsenm added inline comments.
llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
702

If AArch64 doesn't support these without a constant, then they can't be legal. Legality cannot be contextual. This would need to be custom, and lower if not constant to be technically correct

Revision Contents

PathSize
llvm/
lib/
Target/
AArch64/
GISel/
1 line
test/
CodeGen/
AArch64/
GlobalISel/
8 lines

Diff 332805

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

Show First 20 Lines • Show All 693 Lines • ▼ Show 20 LinesAArch64LegalizerInfo::AArch64LegalizerInfo(const AArch64Subtarget &ST)
getActionDefinitionsBuilder(G_VECREDUCE_ADD) getActionDefinitionsBuilder(G_VECREDUCE_ADD)
.legalFor( .legalFor(
{{s8, v16s8}, {s16, v8s16}, {s32, v4s32}, {s32, v2s32}, {s64, v2s64}}) {{s8, v16s8}, {s16, v8s16}, {s32, v4s32}, {s32, v2s32}, {s64, v2s64}})
.lower(); .lower();
getActionDefinitionsBuilder({G_UADDSAT, G_USUBSAT}) getActionDefinitionsBuilder({G_UADDSAT, G_USUBSAT})
.lowerIf([=](const LegalityQuery &Q) { return Q.Types[0].isScalar(); }); .lowerIf([=](const LegalityQuery &Q) { return Q.Types[0].isScalar(); });
getActionDefinitionsBuilder({G_SBFX, G_UBFX}).legalFor({s32, s64});
arsenmUnsubmitted
Not Done
ReplyInline Actions

If AArch64 doesn't support these without a constant, then they can't be legal. Legality cannot be contextual. This would need to be custom, and lower if not constant to be technically correct

arsenm: If AArch64 doesn't support these without a constant, then they can't be legal. Legality cannot…
computeTables(); computeTables();
verify(*ST.getInstrInfo()); verify(*ST.getInstrInfo());
} }
bool AArch64LegalizerInfo::legalizeCustom(LegalizerHelper &Helper, bool AArch64LegalizerInfo::legalizeCustom(LegalizerHelper &Helper,
MachineInstr &MI) const { MachineInstr &MI) const {
MachineIRBuilder &MIRBuilder = Helper.MIRBuilder; MachineIRBuilder &MIRBuilder = Helper.MIRBuilder;
MachineRegisterInfo &MRI = *MIRBuilder.getMRI(); MachineRegisterInfo &MRI = *MIRBuilder.getMRI();
▲ Show 20 Lines • Show All 237 Lines • Show Last 20 Lines

llvm/test/CodeGen/AArch64/GlobalISel/legalizer-info-validation.mir

Show First 20 Lines • Show All 667 Lines • ▼ Show 20 Lines
# DEBUG-NEXT: .. type index coverage check SKIPPED: no rules defined # DEBUG-NEXT: .. type index coverage check SKIPPED: no rules defined
# DEBUG-NEXT: .. imm index coverage check SKIPPED: no rules defined # DEBUG-NEXT: .. imm index coverage check SKIPPED: no rules defined
# DEBUG-NEXT: G_VECREDUCE_UMAX (opcode {{[0-9]+}}): 2 type indices, 0 imm indices # DEBUG-NEXT: G_VECREDUCE_UMAX (opcode {{[0-9]+}}): 2 type indices, 0 imm indices
# DEBUG-NEXT: .. type index coverage check SKIPPED: no rules defined # DEBUG-NEXT: .. type index coverage check SKIPPED: no rules defined
# DEBUG-NEXT: .. imm index coverage check SKIPPED: no rules defined # DEBUG-NEXT: .. imm index coverage check SKIPPED: no rules defined
# DEBUG-NEXT: G_VECREDUCE_UMIN (opcode {{[0-9]+}}): 2 type indices, 0 imm indices # DEBUG-NEXT: G_VECREDUCE_UMIN (opcode {{[0-9]+}}): 2 type indices, 0 imm indices
# DEBUG-NEXT: .. type index coverage check SKIPPED: no rules defined # DEBUG-NEXT: .. type index coverage check SKIPPED: no rules defined
# DEBUG-NEXT: .. imm index coverage check SKIPPED: no rules defined # DEBUG-NEXT: .. imm index coverage check SKIPPED: no rules defined
# DEBUG-NEXT: G_SBFX (opcode {{[0-9]+}}): 1 type index, 0 imm indices
# DEBUG-NEXT: .. the first uncovered type index: 1, OK
# DEBUG-NEXT: .. the first uncovered imm index: 0, OK
# DEBUG-NEXT: G_UBFX (opcode {{[0-9]+}}): 1 type index, 0 imm indices
# DEBUG-NEXT: .. opcode {{[0-9]+}} is aliased to {{[0-9]+}}
# DEBUG-NEXT: .. the first uncovered type index: 1, OK
# DEBUG-NEXT: .. the first uncovered imm index: 0, OK
# CHECK-NOT: ill-defined # CHECK-NOT: ill-defined
--- ---
name: dummy name: dummy
body: | body: |
bb.0: bb.0:
... ...