Skip to content

[GlobalISel] Combine [s,z]ext of undef into 0 #117439

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 3, 2024
Merged

[GlobalISel] Combine [s,z]ext of undef into 0 #117439

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3892315
[GlobalISel] Combine [a,s,z]ext of undef into 0 or undef
tschuett Nov 23, 2024
2fdf196
return true;
tschuett Nov 30, 2024
59422fe
remove return true;
tschuett Nov 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,10 @@ class CombinerHelper {
/// is a legal integer constant type on the target.
bool isConstantLegalOrBeforeLegalizer(const LLT Ty) const;

/// \return true if the combine is running prior to legalization, or if \p Ty
/// is a legal undef type on the target.
bool isUndefLegalOrBeforeLegalizer(const LLT Ty) const;

/// MachineRegisterInfo::replaceRegWith() and inform the observer of the changes
void replaceRegWith(MachineRegisterInfo &MRI, Register FromReg, Register ToReg) const;

Expand Down
28 changes: 26 additions & 2 deletions llvm/include/llvm/Target/GlobalISel/Combine.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ def unary_undef_to_zero: GICombineRule<
// replaced with undef.
def propagate_undef_any_op: GICombineRule<
(defs root:$root),
(match (wip_match_opcode G_ADD, G_FPTOSI, G_FPTOUI, G_SUB, G_XOR, G_TRUNC, G_BITCAST, G_ANYEXT):$root,
(match (wip_match_opcode G_ADD, G_FPTOSI, G_FPTOUI, G_SUB, G_XOR, G_TRUNC, G_BITCAST):$root,
[{ return Helper.matchAnyExplicitUseIsUndef(*${root}); }]),
(apply [{ Helper.replaceInstWithUndef(*${root}); }])>;

Expand Down Expand Up @@ -1857,6 +1857,27 @@ class integer_of_opcode<Instruction castOpcode> : GICombineRule <

def integer_of_truncate : integer_of_opcode<G_TRUNC>;

def anyext_undef: GICombineRule<
(defs root:$root),
(match (G_IMPLICIT_DEF $undef),
(G_ANYEXT $root, $undef):$Aext,
[{ return Helper.isUndefLegalOrBeforeLegalizer(MRI.getType(${Aext}->getOperand(0).getReg())); }]),
(apply [{ Helper.replaceInstWithUndef(*${Aext}); }])>;
Copy link
Contributor

@aemerson aemerson Nov 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary, it was fine where it was in propagate_undef_any_op.

There are two possible situations w.r.t legality here:

  1. We're before legalizer, in which case nothing matters and we can run the combine.
  2. We're post-legalizer. However for undef, since we already agreed that having any legal register definition must imply that G_IMPLICIT_DEF is also legal, then we don't need to check for legality here anyway, since G_ANYEXT defines a vreg with the same type as the new undef instruction.

Copy link
Author

@tschuett tschuett Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The law hasn't been written down yet and the legalizers disagree with that statement. For the moment, the legality is safer.

It would be really confusing if match returns true without any documentation or hints.

Copy link
Contributor

@aemerson aemerson Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make a doc update soon if it's absolutely necessary for you to do the right thing.

Copy link
Author

@tschuett tschuett Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For AArch64, scalable vectors are legal in some positions, but G_IMPLICIT_DEF is not legal for scalable vectors.

Copy link
Contributor

@aemerson aemerson Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's understandable since scalable vectors are not implemented at all.

Copy link
Contributor

@aemerson aemerson Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc update PR: #117609

It turns out we had something almost describing what we wanted anyway.

Copy link
Author

@tschuett tschuett Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's understandable since scalable vectors are not implemented at all.

That is not true. There are running GlobalISel tests with SVE every time you invoke ninja check-llvm-codegen-aarch64.

Copy link
Contributor

@aemerson aemerson Nov 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure ok, we have a tiny amount of support. We still have 99% to go.


def zext_undef: GICombineRule<
(defs root:$root),
(match (G_IMPLICIT_DEF $undef),
(G_ZEXT $root, $undef):$Zext,
[{ return Helper.isConstantLegalOrBeforeLegalizer(MRI.getType(${Zext}->getOperand(0).getReg())); }]),
(apply [{ Helper.replaceInstWithConstant(*${Zext}, 0); }])>;

def sext_undef: GICombineRule<
(defs root:$root),
(match (G_IMPLICIT_DEF $undef),
(G_SEXT $root, $undef):$Sext,
[{ return Helper.isConstantLegalOrBeforeLegalizer(MRI.getType(${Sext}->getOperand(0).getReg())); }]),
(apply [{ Helper.replaceInstWithConstant(*${Sext}, 0); }])>;

def cast_of_cast_combines: GICombineGroup<[
truncate_of_zext,
truncate_of_sext,
Expand All @@ -1882,7 +1903,10 @@ def cast_combines: GICombineGroup<[
narrow_binop_and,
narrow_binop_or,
narrow_binop_xor,
integer_of_truncate
integer_of_truncate,
anyext_undef,
sext_undef,
zext_undef
]>;

def canonicalize_icmp : GICombineRule<
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,10 @@ bool CombinerHelper::isConstantLegalOrBeforeLegalizer(const LLT Ty) const {
isLegal({TargetOpcode::G_CONSTANT, {EltTy}});
}

bool CombinerHelper::isUndefLegalOrBeforeLegalizer(const LLT Ty) const {
return isPreLegalize() || isLegal({TargetOpcode::G_IMPLICIT_DEF, {Ty}});
}

void CombinerHelper::replaceRegWith(MachineRegisterInfo &MRI, Register FromReg,
Register ToReg) const {
Observer.changingAllUsesOfReg(MRI, FromReg);
Expand Down
52 changes: 52 additions & 0 deletions llvm/test/CodeGen/AArch64/GlobalISel/combine-cast.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,3 +217,55 @@ body: |
%large:_(<2 x s64>) = G_ANYEXT %bv(<2 x s32>)
$q0 = COPY %large(<2 x s64>)
$d0 = COPY %bv(<2 x s32>)
...
---
name: test_combine_anyext_undef
legalized: true
body: |
bb.1:
; CHECK-PRE-LABEL: name: test_combine_anyext_undef
; CHECK-PRE: %aext:_(s64) = G_IMPLICIT_DEF
; CHECK-PRE-NEXT: $x0 = COPY %aext(s64)
;
; CHECK-POST-LABEL: name: test_combine_anyext_undef
; CHECK-POST: %undef:_(s32) = G_IMPLICIT_DEF
; CHECK-POST-NEXT: %aext:_(s64) = G_ANYEXT %undef(s32)
; CHECK-POST-NEXT: $x0 = COPY %aext(s64)
%undef:_(s32) = G_IMPLICIT_DEF
%aext:_(s64) = G_ANYEXT %undef(s32)
$x0 = COPY %aext(s64)
...
---
name: test_combine_sext_undef
legalized: true
body: |
bb.1:
; CHECK-PRE-LABEL: name: test_combine_sext_undef
; CHECK-PRE: %sext:_(s64) = G_CONSTANT i64 0
; CHECK-PRE-NEXT: $x0 = COPY %sext(s64)
;
; CHECK-POST-LABEL: name: test_combine_sext_undef
; CHECK-POST: %undef:_(s32) = G_IMPLICIT_DEF
; CHECK-POST-NEXT: %sext:_(s64) = G_SEXT %undef(s32)
; CHECK-POST-NEXT: $x0 = COPY %sext(s64)
%undef:_(s32) = G_IMPLICIT_DEF
%sext:_(s64) = G_SEXT %undef(s32)
$x0 = COPY %sext(s64)
...
---
name: test_combine_zext_undef
legalized: true
body: |
bb.1:
; CHECK-PRE-LABEL: name: test_combine_zext_undef
; CHECK-PRE: %zext:_(s64) = G_CONSTANT i64 0
; CHECK-PRE-NEXT: $x0 = COPY %zext(s64)
;
; CHECK-POST-LABEL: name: test_combine_zext_undef
; CHECK-POST: %undef:_(s32) = G_IMPLICIT_DEF
; CHECK-POST-NEXT: %zext:_(s64) = G_ZEXT %undef(s32)
; CHECK-POST-NEXT: $x0 = COPY %zext(s64)
%undef:_(s32) = G_IMPLICIT_DEF
%zext:_(s64) = G_ZEXT %undef(s32)
$x0 = COPY %zext(s64)
...
15 changes: 4 additions & 11 deletions llvm/test/CodeGen/AArch64/extract-vector-elt.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,10 @@
; CHECK-GI-NEXT: warning: Instruction selection used fallback path for extract_v4i32_vector_extract_const

define i64 @extract_v2i64_undef_index(<2 x i64> %a, i32 %c) {
; CHECK-SD-LABEL: extract_v2i64_undef_index:
; CHECK-SD: // %bb.0: // %entry
; CHECK-SD-NEXT: fmov x0, d0
; CHECK-SD-NEXT: ret
;
; CHECK-GI-LABEL: extract_v2i64_undef_index:
; CHECK-GI: // %bb.0: // %entry
; CHECK-GI-NEXT: str q0, [sp, #-16]!
; CHECK-GI-NEXT: .cfi_def_cfa_offset 16
; CHECK-GI-NEXT: ldr x0, [sp], #16
; CHECK-GI-NEXT: ret
; CHECK-LABEL: extract_v2i64_undef_index:
; CHECK: // %bb.0: // %entry
; CHECK-NEXT: fmov x0, d0
; CHECK-NEXT: ret
entry:
%d = extractelement <2 x i64> %a, i32 undef
ret i64 %d
Expand Down
6 changes: 2 additions & 4 deletions llvm/test/CodeGen/AMDGPU/GlobalISel/combine-amdgpu-cvt-f32-ubyte.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,8 +261,7 @@ body: |
; CHECK-LABEL: name: cvt_f32_ubyte0_zext_lshr_16
; CHECK: liveins: $vgpr0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: %shift:_(s16) = G_IMPLICIT_DEF
; CHECK-NEXT: %zext:_(s32) = G_ZEXT %shift(s16)
; CHECK-NEXT: %zext:_(s32) = G_CONSTANT i32 0
; CHECK-NEXT: %result:_(s32) = G_AMDGPU_CVT_F32_UBYTE0 %zext
; CHECK-NEXT: $vgpr0 = COPY %result(s32)
%arg:_(s32) = COPY $vgpr0
Expand All @@ -284,8 +283,7 @@ body: |
; CHECK-LABEL: name: cvt_f32_ubyte0_zext_lshr_24
; CHECK: liveins: $vgpr0
; CHECK-NEXT: {{ $}}
; CHECK-NEXT: %shift:_(s16) = G_IMPLICIT_DEF
; CHECK-NEXT: %zext:_(s32) = G_ZEXT %shift(s16)
; CHECK-NEXT: %zext:_(s32) = G_CONSTANT i32 0
; CHECK-NEXT: %result:_(s32) = G_AMDGPU_CVT_F32_UBYTE0 %zext
; CHECK-NEXT: $vgpr0 = COPY %result(s32)
%arg:_(s32) = COPY $vgpr0
Expand Down
7 changes: 1 addition & 6 deletions llvm/test/CodeGen/AMDGPU/shrink-add-sub-constant.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4074,14 +4074,12 @@ define amdgpu_kernel void @v_test_v2i16_x_add_undef_neg32(ptr addrspace(1) %out,
; VI-GISEL-NEXT: v_addc_u32_e32 v1, vcc, 0, v1, vcc
; VI-GISEL-NEXT: flat_load_dword v3, v[0:1]
; VI-GISEL-NEXT: v_mov_b32_e32 v0, s0
; VI-GISEL-NEXT: v_mov_b32_e32 v1, s1
; VI-GISEL-NEXT: v_add_u32_e32 v0, vcc, v0, v2
; VI-GISEL-NEXT: v_not_b32_e32 v2, 31
; VI-GISEL-NEXT: v_mov_b32_e32 v1, s1
; VI-GISEL-NEXT: s_and_b32 s0, 0xffff, s0
; VI-GISEL-NEXT: v_addc_u32_e32 v1, vcc, 0, v1, vcc
; VI-GISEL-NEXT: s_waitcnt vmcnt(0)
; VI-GISEL-NEXT: v_add_u16_sdwa v2, v3, v2 dst_sel:WORD_1 dst_unused:UNUSED_PAD src0_sel:WORD_1 src1_sel:DWORD
; VI-GISEL-NEXT: v_or_b32_e32 v2, s0, v2
; VI-GISEL-NEXT: flat_store_dword v[0:1], v2
; VI-GISEL-NEXT: s_endpgm
;
Expand Down Expand Up @@ -4191,15 +4189,12 @@ define amdgpu_kernel void @v_test_v2i16_x_add_neg32_undef(ptr addrspace(1) %out,
; VI-GISEL-NEXT: v_add_u32_e32 v0, vcc, v0, v2
; VI-GISEL-NEXT: v_addc_u32_e32 v1, vcc, 0, v1, vcc
; VI-GISEL-NEXT: flat_load_dword v3, v[0:1]
; VI-GISEL-NEXT: s_and_b32 s2, 0xffff, s0
; VI-GISEL-NEXT: v_mov_b32_e32 v0, s0
; VI-GISEL-NEXT: v_mov_b32_e32 v1, s1
; VI-GISEL-NEXT: v_add_u32_e32 v0, vcc, v0, v2
; VI-GISEL-NEXT: s_lshl_b32 s0, s2, 16
; VI-GISEL-NEXT: v_addc_u32_e32 v1, vcc, 0, v1, vcc
; VI-GISEL-NEXT: s_waitcnt vmcnt(0)
; VI-GISEL-NEXT: v_add_u16_e32 v2, 0xffe0, v3
; VI-GISEL-NEXT: v_or_b32_e32 v2, s0, v2
; VI-GISEL-NEXT: flat_store_dword v[0:1], v2
; VI-GISEL-NEXT: s_endpgm
;
Expand Down
Loading