Skip to content

[GlobalISel] Combine G_ZEXT of undef -> 0 #113764

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

Closed
wants to merge 2 commits into from
Closed

[GlobalISel] Combine G_ZEXT of undef -> 0 #113764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1aaa61
[GlobalISel] Combine G_ZEXT of undef -> 0
tschuett Oct 26, 2024
6a90330
address review comments
tschuett Oct 26, 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
2 changes: 1 addition & 1 deletion llvm/include/llvm/Target/GlobalISel/Combine.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,7 +412,7 @@ def binop_right_undef_to_undef: GICombineRule<

def unary_undef_to_zero: GICombineRule<
(defs root:$root),
(match (wip_match_opcode G_ABS):$root,
(match (wip_match_opcode G_ABS, G_ZEXT):$root,
[{ return Helper.matchOperandIsUndef(*${root}, 1); }]),
(apply [{ Helper.replaceInstWithConstant(*${root}, 0); }])>;

Expand Down
7 changes: 5 additions & 2 deletions llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2916,8 +2916,11 @@ void CombinerHelper::replaceInstWithFConstant(MachineInstr &MI, double C) {

void CombinerHelper::replaceInstWithConstant(MachineInstr &MI, int64_t C) {
assert(MI.getNumDefs() == 1 && "Expected only one def?");
Builder.buildConstant(MI.getOperand(0), C);
MI.eraseFromParent();
LLT DstTy = MRI.getType(MI.getOperand(0).getReg());
if (isConstantLegalOrBeforeLegalizer(DstTy)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess I'd expect this to be checked in the match rule

Copy link
Author

Choose a reason for hiding this comment

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

It isn't. See above.

Copy link
Author

Choose a reason for hiding this comment

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

Just because a G_ZEXT is sitting on a register doesn't imply that a G_CONSTANT is legal on that register. GlobalISel is for me for weird hardware.

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't, but should it

Copy link
Author

Choose a reason for hiding this comment

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

With the current code, we can build illegal MIR after the legalizer.

Copy link
Author

Choose a reason for hiding this comment

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

One option is to move away from the generic matchers Helper.matchOperandIsUndef to specific matchers for unary_undef_to_zero, i.e., Helper.matchUndefToZero.

But I really really hate to have a function that builds something and I have to search around if and where the legality check is.

Copy link
Contributor

Choose a reason for hiding this comment

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

One option is to move away from the generic matchers Helper.matchOperandIsUndef to specific matchers for unary_undef_to_zero, i.e., Helper.matchUndefToZero.

Why not just check in the matcher C++ code? The matcher already knows what the intended output looks like, it just defers the actual mutation to the applicator.

But I really really hate to have a function that builds something and I have to search around if and where the legality check is.

If I understand right you're basically saying you don't like the combiner infrastructure as it was designed, because that's how it's supposed to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a need for Helper.matchOperandIsUndef any longer. You can directly match a G_IMPLICIT_DEF input in tablegen. The match predicate code would then only need to check the legality of the new G_IMPLICIT_DEF or G_CONSTANT.

The legality check would always be in the matcher, there shouldn't be any searching to do.

Copy link
Author

Choose a reason for hiding this comment

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

I prefer my solution. The legality check and the build are close together and easy to reason about.

Firstly, Helper.matchOperandIsUndef does not know whether we are combining into undef or constants. How can it reason about legality of uncertainty? Secondly, legality checks and build are spread wide afar, which makes it hard to reason about correctness.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then the utility of matchOperandIsUndef is limited and should not be used, or supplemented with a legality check

Builder.buildConstant(MI.getOperand(0), C);
MI.eraseFromParent();
}
}

void CombinerHelper::replaceInstWithConstant(MachineInstr &MI, APInt C) {
Expand Down
17 changes: 17 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,20 @@ 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_zext_undef
legalized: true
body: |
bb.1:
; CHECK-LABEL: name: test_combine_zext_undef
; CHECK: %undef:_(<2 x s32>) = G_IMPLICIT_DEF
; CHECK-NEXT: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
; CHECK-NEXT: %large:_(<2 x s64>) = G_BUILD_VECTOR [[C]](s64), [[C]](s64)
; CHECK-NEXT: $q0 = COPY %large(<2 x s64>)
; CHECK-NEXT: $d0 = COPY %undef(<2 x s32>)
%undef:_(<2 x s32>) = G_IMPLICIT_DEF
%large:_(<2 x s64>) = G_ZEXT %undef(<2 x s32>)
$q0 = COPY %large(<2 x s64>)
$d0 = COPY %undef(<2 x s32>)
...
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 @@ -3962,14 +3962,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 @@ -4079,15 +4077,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