Skip to content

[GlobalISel] Combine G_MERGE_VALUES of undef #113381

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 3 commits into from
Closed

[GlobalISel] Combine G_MERGE_VALUES of undef #113381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7552f2
[GlobalISel] Combine G_MERGE_VALUES of undef
tschuett Oct 22, 2024
9f29ffa
fix unit tests
tschuett Oct 23, 2024
a5f8edf
fix typos
tschuett Oct 23, 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
3 changes: 2 additions & 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 @@ -420,7 +420,8 @@ 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,
G_ANYEXT, G_MERGE_VALUES):$root,
Copy link
Collaborator

@davemgreen davemgreen Oct 23, 2024

Choose a reason for hiding this comment

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

I feel this should be in propagate_undef_all_ops.

Copy link
Author

@tschuett tschuett Oct 23, 2024

Choose a reason for hiding this comment

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

Assuming G_MERGE_VALUES (5, undef), I would expect the result to be undef.

Copy link
Collaborator

@davemgreen davemgreen Oct 23, 2024

Choose a reason for hiding this comment

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

It should be <5,undef> if the output was a vector type. If it is merging scalars then I'm not sure how undef propagation in gisel is expected to work. As far as I understand it is not poison, and this case sounds more like an anyextend.

Copy link
Collaborator

@davemgreen davemgreen Oct 23, 2024

Choose a reason for hiding this comment

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

bswap_i16_to_i128_anyext for example looks like it should end up with something from the input in the top bits, not just zeros.

Copy link
Author

@tschuett tschuett Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

llvm-project/llvm/lib/CodeGen/MachineVerifier.cpp

Line 1457 in ae618d3

// G_MERGE_VALUES should only be used to merge scalars into a larger scalar,

It merges several scalars into a larger scalar. This is the reason why I argue for propagate_undef_any_op.

Copy link
Author

@tschuett tschuett Oct 23, 2024

Choose a reason for hiding this comment

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

; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
; RUN: llc %s -stop-after=legalizer -verify-machineinstrs -mtriple aarch64-apple-darwin -global-isel -o - 2>&1 | FileCheck %s

; The zext here is optimised to an any_extend during isel..
define i128 @bswap_i16_to_i128_anyext(i16 %a) {
  ; CHECK-LABEL: name: bswap_i16_to_i128_anyext
  ; CHECK: bb.1 (%ir-block.0):
  ; CHECK-NEXT:   liveins: $w0
  ; CHECK-NEXT: {{  $}}
  ; CHECK-NEXT:   [[COPY:%[0-9]+]]:_(s32) = COPY $w0
  ; CHECK-NEXT:   [[BSWAP:%[0-9]+]]:_(s32) = G_BSWAP [[COPY]]
  ; CHECK-NEXT:   [[C:%[0-9]+]]:_(s64) = G_CONSTANT i64 16
  ; CHECK-NEXT:   [[LSHR:%[0-9]+]]:_(s32) = G_LSHR [[BSWAP]], [[C]](s64)
  ; CHECK-NEXT:   [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
  ; CHECK-NEXT:   [[MV:%[0-9]+]]:_(s64) = G_MERGE_VALUES [[LSHR]](s32), [[DEF]](s32)
  ; CHECK-NEXT:   [[C1:%[0-9]+]]:_(s64) = G_CONSTANT i64 65535
  ; CHECK-NEXT:   [[C2:%[0-9]+]]:_(s64) = G_CONSTANT i64 0
  ; CHECK-NEXT:   [[AND:%[0-9]+]]:_(s64) = G_AND [[MV]], [[C1]]
  ; CHECK-NEXT:   [[C3:%[0-9]+]]:_(s64) = G_CONSTANT i64 48
  ; CHECK-NEXT:   [[SHL:%[0-9]+]]:_(s64) = G_SHL [[AND]], [[C3]](s64)
  ; CHECK-NEXT:   $x0 = COPY [[C2]](s64)
  ; CHECK-NEXT:   $x1 = COPY [[SHL]](s64)
  ; CHECK-NEXT:   RET_ReallyLR implicit $x0, implicit $x1
   %3 = call i16 @llvm.bswap.i16(i16 %a)
    %4 = zext i16 %3 to i128
    %5 = shl i128 %4, 112
    ret i128 %5
}
  • We have a G_MERGE_VALUES of undef
  • The new undef feeds the G_AND
  • The new undef of the G_AND feeds the G_SHL.
  • The G_SHL of the new undef becomes zero.
def binop_left_undef_to_zero: GICombineRule<
  (defs root:$root),
  (match (wip_match_opcode G_SHL, G_UDIV, G_UREM):$root,
         [{ return Helper.matchOperandIsUndef(*${root}, 1); }]),
  (apply [{ Helper.replaceInstWithConstant(*${root}, 0); }])>;

The result is zero.

Copy link
Member

@lenary lenary Oct 23, 2024

Choose a reason for hiding this comment

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

Right now with this optimisation, the function returns 128-bit zero, but if we try to validate the equivalent optimisation just with LLVM IR (i.e., replacing the whole function with ret i128 0, alive2 really complains, and shows an input where the return values don't actually match according to the IR: https://alive2.llvm.org/ce/z/8gdDkw

I believe @regehr also has a tool to do LLVM IR to AArch64 Assembly translation validation, which I believe would also agree that this optimisation, as implemented, is wrong.

Copy link
Author

@tschuett tschuett Oct 23, 2024

Choose a reason for hiding this comment

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

Independent of whether the optimization is valid, the GMIR is legal for AArch64. The bswap is on 32-bit and not on 16-bit. There are operations in the GMIR that are not in LLVM-IR. Another question is why and where are we creating G_MERGE_VALUES of undef.

Copy link
Author

@tschuett tschuett Oct 23, 2024

Choose a reason for hiding this comment

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

If you try the ag command in the heading, the pattern occurs several times in existing tests.

The other question is how to interpret an G_MERGE_VALUES of undef.

 %0:(s32) = G_MERGE_VALUES %bits_0_7:(s8), %bits_8_15:(s8),  %bits_16_23:(s8), %bits_24_31:(s8)

What does it mean if %bits_24_31 is undef? Is a subrange of the output invalid or is the complete output invalid? What happens if the output is used by other operations? In the exampled MV is used by the G_AND. Is a subrange of MV invalid?

Copy link
Member

@lenary lenary Oct 23, 2024

Choose a reason for hiding this comment

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

I have no problems with how this has been legalized.

This patch proposes an optimisation that relies on specific semantics of G_MERGE_VALUES and undef - that if any inputs are undef, then the output must be undef. This optimisation produces miscompiles, as the alive2 link shows - so the optimisation must not be correct.

I think that David is right, and the output of G_MERGE_VALUES should only be undef if all inputs are undef (not if any are undef). This reasoning makes sense to me because if you are assembling a wide value by concatenating some defined bits and some undefined bits, then that wide value must still have some defined bits (not be all undefined bits). If you are assembling a wide value by concatenating only undefined bits, then it stands to reason that all the outputs are undefined.

[{ return Helper.matchAnyExplicitUseIsUndef(*${root}); }]),
(apply [{ Helper.replaceInstWithUndef(*${root}); }])>;

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 @@ -2935,8 +2935,11 @@ void CombinerHelper::replaceInstWithFConstant(MachineInstr &MI,

void CombinerHelper::replaceInstWithUndef(MachineInstr &MI) {
assert(MI.getNumDefs() == 1 && "Expected only one def?");
Builder.buildUndef(MI.getOperand(0));
MI.eraseFromParent();
if (isLegalOrBeforeLegalizer({TargetOpcode::G_IMPLICIT_DEF,
{MRI.getType(MI.getOperand(0).getReg())}})) {
Builder.buildUndef(MI.getOperand(0));
MI.eraseFromParent();
}
}

bool CombinerHelper::matchSimplifyAddToSub(
Expand Down
12 changes: 10 additions & 2 deletions llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,16 +159,17 @@ void GISelKnownBits::computeKnownBitsImpl(Register R, KnownBits &Known,
}
#endif

unsigned BitWidth = DstTy.getScalarSizeInBits();

// Handle the case where this is called on a register that does not have a
// type constraint (i.e. it has a register class constraint instead). This is
// unlikely to occur except by looking through copies but it is possible for
// the initial register being queried to be in this state.
if (!DstTy.isValid()) {
Known = KnownBits();
Known = KnownBits(BitWidth); // Don't know anything
return;
}

unsigned BitWidth = DstTy.getScalarSizeInBits();
auto CacheEntry = ComputeKnownBitsCache.find(R);
if (CacheEntry != ComputeKnownBitsCache.end()) {
Known = CacheEntry->second;
Expand Down Expand Up @@ -200,6 +201,8 @@ void GISelKnownBits::computeKnownBitsImpl(Register R, KnownBits &Known,
TL.computeKnownBitsForTargetInstr(*this, R, Known, DemandedElts, MRI,
Depth);
break;
case TargetOpcode::G_IMPLICIT_DEF:
break;
case TargetOpcode::G_BUILD_VECTOR: {
// Collect the known bits that are shared by every demanded vector element.
Known.Zero.setAllBits(); Known.One.setAllBits();
Expand Down Expand Up @@ -579,13 +582,16 @@ void GISelKnownBits::computeKnownBitsImpl(Register R, KnownBits &Known,
break;
}
case TargetOpcode::G_SBFX: {
// FIXME: the three parameters do not have the same types and bitwidths.
break;
KnownBits SrcOpKnown, OffsetKnown, WidthKnown;
computeKnownBitsImpl(MI.getOperand(1).getReg(), SrcOpKnown, DemandedElts,
Depth + 1);
computeKnownBitsImpl(MI.getOperand(2).getReg(), OffsetKnown, DemandedElts,
Depth + 1);
computeKnownBitsImpl(MI.getOperand(3).getReg(), WidthKnown, DemandedElts,
Depth + 1);

Known = extractBits(BitWidth, SrcOpKnown, OffsetKnown, WidthKnown);
// Sign extend the extracted value using shift left and arithmetic shift
// right.
Expand Down Expand Up @@ -627,6 +633,8 @@ void GISelKnownBits::computeKnownBitsImpl(Register R, KnownBits &Known,
}
}

assert(Known.getBitWidth() == BitWidth && "Bit widths must be the same");

LLVM_DEBUG(dumpResult(MI, Known, Depth));

// Update the cache.
Expand Down
25 changes: 9 additions & 16 deletions llvm/test/CodeGen/AArch64/GlobalISel/combine-unmerge.mir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@ body: |
bb.1:
; CHECK-LABEL: name: test_combine_unmerge_merge
; CHECK: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: [[DEF1:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: $w0 = COPY [[DEF]](s32)
; CHECK-NEXT: $w1 = COPY [[DEF1]](s32)
; CHECK-NEXT: $w1 = COPY [[DEF]](s32)
%0:_(s32) = G_IMPLICIT_DEF
%1:_(s32) = G_IMPLICIT_DEF
%2:_(s64) = G_MERGE_VALUES %0(s32), %1(s32)
Expand All @@ -30,11 +29,9 @@ body: |
bb.1:
; CHECK-LABEL: name: test_combine_unmerge_merge_3ops
; CHECK: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: [[DEF1:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: [[DEF2:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: $w0 = COPY [[DEF]](s32)
; CHECK-NEXT: $w1 = COPY [[DEF1]](s32)
; CHECK-NEXT: $w2 = COPY [[DEF2]](s32)
; CHECK-NEXT: $w1 = COPY [[DEF]](s32)
; CHECK-NEXT: $w2 = COPY [[DEF]](s32)
%0:_(s32) = G_IMPLICIT_DEF
%1:_(s32) = G_IMPLICIT_DEF
%5:_(s32) = G_IMPLICIT_DEF
Expand Down Expand Up @@ -115,9 +112,8 @@ body: |
bb.1:
; CHECK-LABEL: name: test_combine_unmerge_bitcast_merge
; CHECK: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: [[DEF1:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: $w0 = COPY [[DEF]](s32)
; CHECK-NEXT: $w1 = COPY [[DEF1]](s32)
; CHECK-NEXT: $w1 = COPY [[DEF]](s32)
%0:_(s32) = G_IMPLICIT_DEF
%1:_(s32) = G_IMPLICIT_DEF
%2:_(s64) = G_MERGE_VALUES %0(s32), %1(s32)
Expand All @@ -135,14 +131,11 @@ name: test_combine_unmerge_merge_incompatible_types
body: |
bb.1:
; CHECK-LABEL: name: test_combine_unmerge_merge_incompatible_types
; CHECK: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: [[DEF1:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
; CHECK-NEXT: [[MV:%[0-9]+]]:_(s64) = G_MERGE_VALUES [[DEF]](s32), [[DEF1]](s32)
; CHECK-NEXT: [[UV:%[0-9]+]]:_(s16), [[UV1:%[0-9]+]]:_(s16), [[UV2:%[0-9]+]]:_(s16), [[UV3:%[0-9]+]]:_(s16) = G_UNMERGE_VALUES [[MV]](s64)
; CHECK-NEXT: $h0 = COPY [[UV]](s16)
; CHECK-NEXT: $h1 = COPY [[UV1]](s16)
; CHECK-NEXT: $h2 = COPY [[UV2]](s16)
; CHECK-NEXT: $h3 = COPY [[UV3]](s16)
; CHECK: [[DEF:%[0-9]+]]:_(s16) = G_IMPLICIT_DEF
; CHECK-NEXT: $h0 = COPY [[DEF]](s16)
; CHECK-NEXT: $h1 = COPY [[DEF]](s16)
; CHECK-NEXT: $h2 = COPY [[DEF]](s16)
; CHECK-NEXT: $h3 = COPY [[DEF]](s16)
%0:_(s32) = G_IMPLICIT_DEF
%1:_(s32) = G_IMPLICIT_DEF
%2:_(s64) = G_MERGE_VALUES %0(s32), %1(s32)
Expand Down
7 changes: 1 addition & 6 deletions llvm/test/CodeGen/AArch64/bswap.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,8 @@ define i128 @bswap_i16_to_i128_anyext(i16 %a) {
;
; CHECK-GI-LABEL: bswap_i16_to_i128_anyext:
; CHECK-GI: // %bb.0:
; CHECK-GI-NEXT: mov w8, w0
; CHECK-GI-NEXT: mov x0, xzr
; CHECK-GI-NEXT: rev w8, w8
; CHECK-GI-NEXT: lsr w8, w8, #16
; CHECK-GI-NEXT: bfi x8, x8, #32, #32
; CHECK-GI-NEXT: and x8, x8, #0xffff
; CHECK-GI-NEXT: lsl x1, x8, #48
; CHECK-GI-NEXT: mov x1, xzr
; CHECK-GI-NEXT: ret
%3 = call i16 @llvm.bswap.i16(i16 %a)
%4 = zext i16 %3 to i128
Expand Down
Loading
Loading