This is an archive of the discontinued LLVM Phabricator instance.

Differential D151803

[GlobalIsel][X86] Legalize G_BSWAP II
AbandonedPublic

Authored by tschuett on May 31 2023, 6:35 AM.

Details

Reviewers
RKSimon
arsenm
Summary

Note the workaround for 32 bit

Diff Detail

Event Timeline

tschuett created this revision.May 31 2023, 6:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 6:35 AM
Herald added subscribers: pengfei, hiraditya. · View Herald Transcript
tschuett requested review of this revision.May 31 2023, 6:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2023, 6:35 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
tschuett added reviewers: RKSimon, arsenm.May 31 2023, 6:35 AM
Herald added a subscriber: wdng. · View Herald TranscriptMay 31 2023, 6:35 AM
Harbormaster completed remote builds in B235556: Diff 527016.May 31 2023, 7:25 AM
arsenm added inline comments.Jun 1 2023, 10:18 AM
llvm/test/CodeGen/X86/GlobalISel/legalize-bswap.mir
76

Needs some non-power-of-2 tests

tschuett updated this revision to Diff 527492.Jun 1 2023, 10:32 AM

add s17

Harbormaster completed remote builds in B235896: Diff 527492.Jun 1 2023, 10:32 AM
arsenm added inline comments.Jun 1 2023, 10:42 AM
llvm/test/CodeGen/X86/GlobalISel/legalize-bswap.mir
9

I think something went wrong with the latest diff.

Anyway, you need a non-power-of-2 test in-between the clamped range. That's the case which wouldn't be caught if you dropped the widenScalarToNextPow2 part

tschuett updated this revision to Diff 527498.Jun 1 2023, 10:43 AM

fix I

Harbormaster completed remote builds in B235901: Diff 527498.Jun 1 2023, 10:43 AM
tschuett updated this revision to Diff 527500.Jun 1 2023, 10:46 AM

second attempt

Harbormaster completed remote builds in B235903: Diff 527500.Jun 1 2023, 10:47 AM
RKSimon added inline comments.Jun 1 2023, 10:52 AM
llvm/lib/Target/X86/X86LegalizerInfo.cpp
105

Add the sMax variable from D151878 instead of s64

127

If you're going to use legalIf, why not merge them (the plan is to get rid of the if (Subtarget.is64Bit()) if-else entirely):

return Query.Types[0] == s32 || (Subtarget.is64Bit() && Query.Types[0] == s64);
tschuett abandoned this revision.Jun 2 2023, 1:22 AM

superseded by https://reviews.llvm.org/D151915

Revision Contents

PathSize
llvm/
lib/
Target/
X86/
2 lines
test/
CodeGen/
X86/
GlobalISel/
2 lines

Diff 527492

llvm/lib/Target/X86/X86LegalizerInfo.cpp

Show First 20 Lines • Show All 96 Lines • ▼ Show 20 LinesX86LegalizerInfo::X86LegalizerInfo(const X86Subtarget &STI,
getActionDefinitionsBuilder({G_MEMCPY, G_MEMMOVE, G_MEMSET}).libcall(); getActionDefinitionsBuilder({G_MEMCPY, G_MEMMOVE, G_MEMSET}).libcall();
if (Subtarget.is64Bit()) { if (Subtarget.is64Bit()) {
getActionDefinitionsBuilder(G_BSWAP) getActionDefinitionsBuilder(G_BSWAP)
.legalFor({s32, s64}) .legalFor({s32, s64})
.widenScalarToNextPow2(0, /*Min=*/32) .widenScalarToNextPow2(0, /*Min=*/32)
.clampScalar(0, s32, s64); .clampScalar(0, s32, s64);
RKSimonUnsubmitted
Not Done
ReplyInline Actions

Add the sMax variable from D151878 instead of s64

RKSimon: Add the sMax variable from D151878 instead of s64
if (Subtarget.hasPOPCNT()) { if (Subtarget.hasPOPCNT()) {
// popcount // popcount (POPCNT)
getActionDefinitionsBuilder(G_CTPOP) getActionDefinitionsBuilder(G_CTPOP)
.legalFor({{s16, s16}, {s32, s32}, {s64, s64}}) .legalFor({{s16, s16}, {s32, s32}, {s64, s64}})
.widenScalarToNextPow2(1, /*Min=*/16) .widenScalarToNextPow2(1, /*Min=*/16)
.clampScalar(1, s16, s64); .clampScalar(1, s16, s64);
} }
if (Subtarget.hasLZCNT()) { if (Subtarget.hasLZCNT()) {
// count leading zeros (LZCNT) // count leading zeros (LZCNT)
getActionDefinitionsBuilder(G_CTLZ) getActionDefinitionsBuilder(G_CTLZ)
.legalFor({{s16, s16}, {s32, s32}, {s64, s64}}) .legalFor({{s16, s16}, {s32, s32}, {s64, s64}})
.widenScalarToNextPow2(1, /*Min=*/16) .widenScalarToNextPow2(1, /*Min=*/16)
.clampScalar(1, s16, s64); .clampScalar(1, s16, s64);
} }
} else { // 32-bit } else { // 32-bit
getActionDefinitionsBuilder(G_BSWAP) getActionDefinitionsBuilder(G_BSWAP)
.legalIf([=](const LegalityQuery &Query) { .legalIf([=](const LegalityQuery &Query) {
// workaround for legalFor // workaround for legalFor
return Query.Types[0] == s32; return Query.Types[0] == s32;
}) })
RKSimonUnsubmitted
Not Done
ReplyInline Actions

If you're going to use legalIf, why not merge them (the plan is to get rid of the if (Subtarget.is64Bit()) if-else entirely):

return Query.Types[0] == s32 || (Subtarget.is64Bit() && Query.Types[0] == s64);
RKSimon: If you're going to use legalIf, why not merge them (the plan is to get rid of the if (Subtarget.
.widenScalarToNextPow2(0, /*Min=*/32) .widenScalarToNextPow2(0, /*Min=*/32)
.clampScalar(0, s32, s32); .clampScalar(0, s32, s32);
if (Subtarget.hasPOPCNT()) { if (Subtarget.hasPOPCNT()) {
// popcount // popcount
getActionDefinitionsBuilder(G_CTPOP) getActionDefinitionsBuilder(G_CTPOP)
.legalFor({{s16, s16}, {s32, s32}}) .legalFor({{s16, s16}, {s32, s32}})
.widenScalarToNextPow2(1, /*Min=*/16) .widenScalarToNextPow2(1, /*Min=*/16)
▲ Show 20 Lines • Show All 491 Lines • Show Last 20 Lines

llvm/test/CodeGen/X86/GlobalISel/legalize-bswap.mir

# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py UTC_ARGS: --version 2
# RUN: llc -mtriple=x86_64-linux-gnu -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=X86-64 # RUN: llc -mtriple=x86_64-linux-gnu -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=X86-64
# RUN: llc -mtriple=i386-linux-gnu -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=X86-32 # RUN: llc -mtriple=i386-linux-gnu -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=X86-32
# test BSWAP s16, s32, and s64 # test BSWAP s16, s17, s32, and s64
... ...
--- ---
name: test_bswaps17 name: test_bswaps17
arsenmUnsubmitted
Not Done
ReplyInline Actions

I think something went wrong with the latest diff.

Anyway, you need a non-power-of-2 test in-between the clamped range. That's the case which wouldn't be caught if you dropped the widenScalarToNextPow2 part

arsenm: I think something went wrong with the latest diff. Anyway, you need a non-power-of-2 test in…
body: | body: |
bb.1: bb.1:
; X86-64-LABEL: name: test_bswaps17 ; X86-64-LABEL: name: test_bswaps17
; X86-64: [[DEF:%[0-9]+]]:_(s17) = IMPLICIT_DEF ; X86-64: [[DEF:%[0-9]+]]:_(s17) = IMPLICIT_DEF
; X86-64-NEXT: [[ANYEXT:%[0-9]+]]:_(s32) = G_ANYEXT [[DEF]](s17) ; X86-64-NEXT: [[ANYEXT:%[0-9]+]]:_(s32) = G_ANYEXT [[DEF]](s17)
; X86-64-NEXT: [[BSWAP:%[0-9]+]]:_(s32) = G_BSWAP [[ANYEXT]] ; X86-64-NEXT: [[BSWAP:%[0-9]+]]:_(s32) = G_BSWAP [[ANYEXT]]
; X86-64-NEXT: [[C:%[0-9]+]]:_(s8) = G_CONSTANT i8 15 ; X86-64-NEXT: [[C:%[0-9]+]]:_(s8) = G_CONSTANT i8 15
; X86-64-NEXT: [[LSHR:%[0-9]+]]:_(s32) = G_LSHR [[BSWAP]], [[C]](s8) ; X86-64-NEXT: [[LSHR:%[0-9]+]]:_(s32) = G_LSHR [[BSWAP]], [[C]](s8)
▲ Show 20 Lines • Show All 50 Lines • ▼ Show 20 Lines bb.1:
; X86-32-LABEL: name: test_bswap_s32 ; X86-32-LABEL: name: test_bswap_s32
; X86-32: [[DEF:%[0-9]+]]:_(s32) = IMPLICIT_DEF ; X86-32: [[DEF:%[0-9]+]]:_(s32) = IMPLICIT_DEF
; X86-32-NEXT: [[BSWAP:%[0-9]+]]:_(s32) = G_BSWAP [[DEF]] ; X86-32-NEXT: [[BSWAP:%[0-9]+]]:_(s32) = G_BSWAP [[DEF]]
; X86-32-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY [[BSWAP]](s32) ; X86-32-NEXT: [[COPY:%[0-9]+]]:_(s32) = COPY [[BSWAP]](s32)
; X86-32-NEXT: RET 0, implicit [[COPY]](s32) ; X86-32-NEXT: RET 0, implicit [[COPY]](s32)
%0:_(s32) = IMPLICIT_DEF %0:_(s32) = IMPLICIT_DEF
%1:_(s32) = G_BSWAP %0 %1:_(s32) = G_BSWAP %0
%2:_(s32) = COPY %1(s32) %2:_(s32) = COPY %1(s32)
RET 0, implicit %2 RET 0, implicit %2
arsenmUnsubmitted
Not Done
ReplyInline Actions

Needs some non-power-of-2 tests

arsenm: Needs some non-power-of-2 tests
... ...
--- ---
name: test_bswap_s16 name: test_bswap_s16
body: | body: |
bb.1: bb.1:
; X86-64-LABEL: name: test_bswap_s16 ; X86-64-LABEL: name: test_bswap_s16
; X86-64: [[DEF:%[0-9]+]]:_(s16) = IMPLICIT_DEF ; X86-64: [[DEF:%[0-9]+]]:_(s16) = IMPLICIT_DEF
Show All 20 Lines