This is an archive of the discontinued LLVM Phabricator instance.

Differential D125041

[GlobalISel] Combine G_SHL, G_ASHR, G_SHL of undef shifts to undef.
ClosedPublic

Authored by aemerson on May 5 2022, 1:58 PM.

Details

Reviewers
paquette
arsenm
foad
jroelofs
Commits
rG41fef1044956: [GlobalISel] Combine G_SHL, G_ASHR, G_SHL of undef shifts to undef.
Summary

This matches when SelectionDAG does.

Diff Detail

Event Timeline

aemerson created this revision.May 5 2022, 1:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2022, 1:58 PM
Herald added a subscriber: rovka. · View Herald Transcript
aemerson requested review of this revision.May 5 2022, 1:58 PM
Herald added a subscriber: wdng. · View Herald TranscriptMay 5 2022, 1:58 PM
aemerson retitled this revision from [GlobalISel] Combine G_SHL, G_ASHR, G_SHL of undef shifts undef. to [GlobalISel] Combine G_SHL, G_ASHR, G_SHL of undef shifts to undef..May 5 2022, 1:58 PM
arsenm added inline comments.May 5 2022, 1:59 PM
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-undef.mir
59–60

Should pre-regenerate these checks to include -NEXT

281

Should also test undef lhs?

Harbormaster completed remote builds in B163000: Diff 427440.May 5 2022, 2:23 PM
aemerson updated this revision to Diff 427485.May 5 2022, 4:12 PM

Actually G_SHL with undef LHS needs to be optimized to zero, not undef.

Harbormaster completed remote builds in B163037: Diff 427485.May 5 2022, 5:05 PM
foad added a comment.May 6 2022, 3:51 AM
In D125041#3495365, @aemerson wrote:

Actually G_SHL with undef LHS needs to be optimized to zero, not undef.

I don't understand why that would be true for SHL but not for the other shifts. What are the rules for undef in MIR? Is it like undef in IR, or like poison in IR, or neither?

llvm/include/llvm/Target/GlobalISel/Combine.td
238

I like 'rhs' but all the existing combine names seem to use 'left' and 'right'.

aemerson added a comment.May 6 2022, 12:37 PM
In D125041#3496339, @foad wrote:
In D125041#3495365, @aemerson wrote:

Actually G_SHL with undef LHS needs to be optimized to zero, not undef.

I don't understand why that would be true for SHL but not for the other shifts. What are the rules for undef in MIR? Is it like undef in IR, or like poison in IR, or neither?

I'm not exactly sure of the reasoning, it's just what the IR optimizations and SelectionDAG do. SelectionDAG optimizes the other shifts of undef to undef too.

aemerson updated this revision to Diff 427733.May 6 2022, 1:55 PM
Harbormaster completed remote builds in B163220: Diff 427733.May 6 2022, 3:03 PM
foad added a comment.May 9 2022, 2:02 AM

Actually G_SHL with undef LHS needs to be optimized to zero, not undef.

At the IR level, InstSimplify does this for all shifts (in the absence of exact/nsw/nuw flags). See SimplifyRightShift and SimplifyShlInst.

aemerson added a comment.May 9 2022, 11:06 AM
In D125041#3500128, @foad wrote:

Actually G_SHL with undef LHS needs to be optimized to zero, not undef.

At the IR level, InstSimplify does this for all shifts (in the absence of exact/nsw/nuw flags). See SimplifyRightShift and SimplifyShlInst.

Ok I see. To be more conservative then I'll just fold to undef for the shifts only.

aemerson updated this revision to Diff 428141.May 9 2022, 11:29 AM

Only fold undef shifts, not LHS.

Harbormaster completed remote builds in B163528: Diff 428141.May 9 2022, 2:00 PM
foad accepted this revision.May 13 2022, 4:12 AM

LGTM. (As I understand it the rationale is that if the rhs is undef, you can assume it is a value >= the bitwidth, so the result is undef.)

This revision is now accepted and ready to land.May 13 2022, 4:12 AM
This revision was landed with ongoing or failed builds.May 13 2022, 12:20 PM
Closed by commit rG41fef1044956: [GlobalISel] Combine G_SHL, G_ASHR, G_SHL of undef shifts to undef. (authored by aemerson). · Explain Why
This revision was automatically updated to reflect the committed changes.
aemerson added a commit: rG41fef1044956: [GlobalISel] Combine G_SHL, G_ASHR, G_SHL of undef shifts to undef..

Revision Contents

PathSize
llvm/
include/
llvm/
Target/
GlobalISel/
7 lines
test/
CodeGen/
AArch64/
GlobalISel/
93 lines

Diff 429325

llvm/include/llvm/Target/GlobalISel/Combine.td

Show First 20 Lines • Show All 229 Lines • ▼ Show 20 Linesdef undef_to_negative_one: GICombineRule<
(apply [{ Helper.replaceInstWithConstant(*${root}, -1); }])>; (apply [{ Helper.replaceInstWithConstant(*${root}, -1); }])>;
def binop_left_undef_to_zero: GICombineRule< def binop_left_undef_to_zero: GICombineRule<
(defs root:$root), (defs root:$root),
(match (wip_match_opcode G_SHL):$root, (match (wip_match_opcode G_SHL):$root,
[{ return Helper.matchOperandIsUndef(*${root}, 1); }]), [{ return Helper.matchOperandIsUndef(*${root}, 1); }]),
(apply [{ Helper.replaceInstWithConstant(*${root}, 0); }])>; (apply [{ Helper.replaceInstWithConstant(*${root}, 0); }])>;
def binop_right_undef_to_undef: GICombineRule<
foadUnsubmitted
Not Done
ReplyInline Actions

I like 'rhs' but all the existing combine names seem to use 'left' and 'right'.

foad: I like 'rhs' but all the existing combine names seem to use 'left' and 'right'.
(defs root:$root),
(match (wip_match_opcode G_SHL, G_ASHR, G_LSHR):$root,
[{ return Helper.matchOperandIsUndef(*${root}, 2); }]),
(apply [{ Helper.replaceInstWithUndef(*${root}); }])>;
// Instructions where if any source operand is undef, the instruction can be // Instructions where if any source operand is undef, the instruction can be
// replaced with undef. // replaced with undef.
def propagate_undef_any_op: GICombineRule< def propagate_undef_any_op: GICombineRule<
(defs root:$root), (defs root:$root),
(match (wip_match_opcode G_ADD, G_FPTOSI, G_FPTOUI, G_SUB, G_XOR, G_TRUNC):$root, (match (wip_match_opcode G_ADD, G_FPTOSI, G_FPTOUI, G_SUB, G_XOR, G_TRUNC):$root,
[{ return Helper.matchAnyExplicitUseIsUndef(*${root}); }]), [{ return Helper.matchAnyExplicitUseIsUndef(*${root}); }]),
(apply [{ Helper.replaceInstWithUndef(*${root}); }])>; (apply [{ Helper.replaceInstWithUndef(*${root}); }])>;
▲ Show 20 Lines • Show All 638 Lines • ▼ Show 20 Lines (match (wip_match_opcode G_FSUB):$root,
[{ return Helper.matchCombineFSubFpExtFNegFMulToFMadOrFMA( [{ return Helper.matchCombineFSubFpExtFNegFMulToFMadOrFMA(
*${root}, ${info}); }]), *${root}, ${info}); }]),
(apply [{ Helper.applyBuildFn(*${root}, ${info}); }])>; (apply [{ Helper.applyBuildFn(*${root}, ${info}); }])>;
// FIXME: These should use the custom predicate feature once it lands. // FIXME: These should use the custom predicate feature once it lands.
def undef_combines : GICombineGroup<[undef_to_fp_zero, undef_to_int_zero, def undef_combines : GICombineGroup<[undef_to_fp_zero, undef_to_int_zero,
undef_to_negative_one, undef_to_negative_one,
binop_left_undef_to_zero, binop_left_undef_to_zero,
binop_right_undef_to_undef,
propagate_undef_any_op, propagate_undef_any_op,
propagate_undef_all_ops, propagate_undef_all_ops,
propagate_undef_shuffle_mask, propagate_undef_shuffle_mask,
erase_undef_store, erase_undef_store,
unmerge_undef]>; unmerge_undef]>;
def identity_combines : GICombineGroup<[select_same_val, right_identity_zero, def identity_combines : GICombineGroup<[select_same_val, right_identity_zero,
binop_same_val, binop_left_to_zero, binop_same_val, binop_left_to_zero,
▲ Show 20 Lines • Show All 53 Lines • Show Last 20 Lines

llvm/test/CodeGen/AArch64/GlobalISel/prelegalizercombiner-undef.mir

Show First 20 Lines • Show All 50 Lines • ▼ Show 20 Lines
... ...
--- ---
name: mul_to_zero name: mul_to_zero
alignment: 4 alignment: 4
tracksRegLiveness: true tracksRegLiveness: true
body: | body: |
bb.0: bb.0:
; CHECK-LABEL: name: mul_to_zero ; CHECK-LABEL: name: mul_to_zero
; CHECK: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 0 ; CHECK: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 0
; CHECK-NEXT: $w0 = COPY [[C]](s32) ; CHECK-NEXT: $w0 = COPY [[C]](s32)
arsenmUnsubmitted
Not Done
ReplyInline Actions

Should pre-regenerate these checks to include -NEXT

arsenm: Should pre-regenerate these checks to include -NEXT
; CHECK-NEXT: RET_ReallyLR implicit $w0 ; CHECK-NEXT: RET_ReallyLR implicit $w0
%0:_(s32) = G_CONSTANT i32 10 %0:_(s32) = G_CONSTANT i32 10
%1:_(s32) = G_IMPLICIT_DEF %1:_(s32) = G_IMPLICIT_DEF
%2:_(s32) = G_MUL %0, %1 %2:_(s32) = G_MUL %0, %1
$w0 = COPY %2(s32) $w0 = COPY %2(s32)
RET_ReallyLR implicit $w0 RET_ReallyLR implicit $w0
... ...
▲ Show 20 Lines • Show All 152 Lines • ▼ Show 20 Lines bb.0:
; CHECK-NEXT: [[SHUF:%[0-9]+]]:_(<2 x s32>) = G_SHUFFLE_VECTOR [[DEF]](<2 x s32>), [[COPY]], shufflemask(0, 1) ; CHECK-NEXT: [[SHUF:%[0-9]+]]:_(<2 x s32>) = G_SHUFFLE_VECTOR [[DEF]](<2 x s32>), [[COPY]], shufflemask(0, 1)
; CHECK-NEXT: $d0 = COPY [[SHUF]](<2 x s32>) ; CHECK-NEXT: $d0 = COPY [[SHUF]](<2 x s32>)
; CHECK-NEXT: RET_ReallyLR implicit $d0 ; CHECK-NEXT: RET_ReallyLR implicit $d0
%1:_(<2 x s32>) = G_IMPLICIT_DEF %1:_(<2 x s32>) = G_IMPLICIT_DEF
%2:_(<2 x s32>) = COPY $d0 %2:_(<2 x s32>) = COPY $d0
%0:_(<2 x s32>) = G_SHUFFLE_VECTOR %1(<2 x s32>), %2(<2 x s32>), shufflemask(0, 1) %0:_(<2 x s32>) = G_SHUFFLE_VECTOR %1(<2 x s32>), %2(<2 x s32>), shufflemask(0, 1)
$d0 = COPY %0(<2 x s32>) $d0 = COPY %0(<2 x s32>)
RET_ReallyLR implicit $d0 RET_ReallyLR implicit $d0
...
---
name: shl_undef_rhs
alignment: 4
tracksRegLiveness: true
body: |
bb.0:
; CHECK-LABEL: name: shl_undef_rhs
; CHECK: [[DEF:%[0-9]+]]:_(s64) = G_IMPLICIT_DEF
; CHECK-NEXT: $x0 = COPY [[DEF]](s64)
; CHECK-NEXT: RET_ReallyLR implicit $x0
%0:_(s64) = G_CONSTANT i64 10
%1:_(s64) = G_IMPLICIT_DEF
%2:_(s64) = G_SHL %0, %1
$x0 = COPY %2(s64)
RET_ReallyLR implicit $x0
...
---
name: lshr_undef_rhs
alignment: 4
tracksRegLiveness: true
body: |
bb.0:
; CHECK-LABEL: name: lshr_undef_rhs
; CHECK: [[DEF:%[0-9]+]]:_(s64) = G_IMPLICIT_DEF
; CHECK-NEXT: $x0 = COPY [[DEF]](s64)
; CHECK-NEXT: RET_ReallyLR implicit $x0
%0:_(s64) = G_CONSTANT i64 10
%1:_(s64) = G_IMPLICIT_DEF
%2:_(s64) = G_LSHR %0, %1
$x0 = COPY %2(s64)
RET_ReallyLR implicit $x0
...
---
name: ashr_undef_rhs
alignment: 4
tracksRegLiveness: true
body: |
bb.0:
; CHECK-LABEL: name: ashr_undef_rhs
; CHECK: [[DEF:%[0-9]+]]:_(s64) = G_IMPLICIT_DEF
; CHECK-NEXT: $x0 = COPY [[DEF]](s64)
; CHECK-NEXT: RET_ReallyLR implicit $x0
%0:_(s64) = G_CONSTANT i64 10
%1:_(s64) = G_IMPLICIT_DEF
%2:_(s64) = G_ASHR %0, %1
$x0 = COPY %2(s64)
RET_ReallyLR implicit $x0
...
arsenmUnsubmitted
Not Done
ReplyInline Actions

Should also test undef lhs?

arsenm: Should also test undef lhs?
---
name: lshr_undef_lhs
alignment: 4
tracksRegLiveness: true
body: |
bb.0:
; Optimize these to zero?
; CHECK-LABEL: name: lshr_undef_lhs
; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 10
; CHECK-NEXT: [[DEF:%[0-9]+]]:_(s64) = G_IMPLICIT_DEF
; CHECK-NEXT: [[LSHR:%[0-9]+]]:_(s64) = G_LSHR [[DEF]], [[C]](s64)
; CHECK-NEXT: $x0 = COPY [[LSHR]](s64)
; CHECK-NEXT: RET_ReallyLR implicit $x0
%0:_(s64) = G_CONSTANT i64 10
%1:_(s64) = G_IMPLICIT_DEF
%2:_(s64) = G_LSHR %1, %0
$x0 = COPY %2(s64)
RET_ReallyLR implicit $x0
...
---
name: ashr_undef_lhs
alignment: 4
tracksRegLiveness: true
body: |
bb.0:
; Optimize these to zero?
; CHECK-LABEL: name: ashr_undef_lhs
; CHECK: [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 10
; CHECK-NEXT: [[DEF:%[0-9]+]]:_(s64) = G_IMPLICIT_DEF
; CHECK-NEXT: [[ASHR:%[0-9]+]]:_(s64) = G_ASHR [[DEF]], [[C]](s64)
; CHECK-NEXT: $x0 = COPY [[ASHR]](s64)
; CHECK-NEXT: RET_ReallyLR implicit $x0
%0:_(s64) = G_CONSTANT i64 10
%1:_(s64) = G_IMPLICIT_DEF
%2:_(s64) = G_ASHR %1, %0
$x0 = COPY %2(s64)
RET_ReallyLR implicit $x0
...