Skip to content

[SelectionDAG] Fix incorrect fold condition in foldSetCCWithFunnelShift. #137637

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 2 commits into from
May 12, 2025
Merged

[SelectionDAG] Fix incorrect fold condition in foldSetCCWithFunnelShift. #137637

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
342c7e1
Pre-commit tests.
Ruhung Apr 28, 2025
cd8ae16
[SelectionDAG] Fix incorrect fold condition in foldSetCCWithFunnelShift.
Ruhung Apr 28, 2025
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
7 changes: 5 additions & 2 deletions llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4462,11 +4462,14 @@ static SDValue foldSetCCWithFunnelShift(EVT VT, SDValue N0, SDValue N1,

unsigned BitWidth = N0.getScalarValueSizeInBits();
auto *ShAmtC = isConstOrConstSplat(N0.getOperand(2));
if (!ShAmtC || ShAmtC->getAPIntValue().uge(BitWidth))
if (!ShAmtC)
return SDValue();

uint64_t ShAmt = ShAmtC->getAPIntValue().urem(BitWidth);
Copy link
Contributor

@jayfoad jayfoad May 9, 2025

Choose a reason for hiding this comment

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

Doesn't this need a .getZExtValue to convert from APInt to uint64_t?

Copy link
Collaborator

@RKSimon RKSimon May 9, 2025

Choose a reason for hiding this comment

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

urem(uint64_t) returns a uint64_t

if (ShAmt == 0)
return SDValue();
Copy link
Collaborator

@RKSimon RKSimon May 2, 2025

Choose a reason for hiding this comment

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

Why not:

uint64_t AmtVal = ShAmtC->getAPIntValue().urem(BitWidth);
if (AmtVal == 0)
  return SDValue();

Copy link
Collaborator

@davemgreen davemgreen May 2, 2025

Choose a reason for hiding this comment

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

From my point of view - just because it should not come up from non-canonical code IIUC, and so fixing the bug is the more important issue and trying to handle other cases that could lead to further bugs for little benefit. But it sounds OK so long as the rest of the code handles it correctly.

Copy link
Contributor Author

@Ruhung Ruhung May 3, 2025

Choose a reason for hiding this comment

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

@RKSimon You're right. It's OK to take the modulo first. I've fixed it. Thanks!


// Canonicalize fshr as fshl to reduce pattern-matching.
unsigned ShAmt = ShAmtC->getZExtValue();
if (N0.getOpcode() == ISD::FSHR)
ShAmt = BitWidth - ShAmt;

Expand Down
24 changes: 24 additions & 0 deletions llvm/test/CodeGen/AArch64/setcc-fsh.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,3 +248,27 @@ define i1 @fshl_or_ne_2(i32 %x, i32 %y) {
%r = icmp ne i32 %f, 2
ret i1 %r
}

define i1 @fshr_0_or_eq_0(i16 %x, i16 %y) {
; CHECK-LABEL: fshr_0_or_eq_0:
; CHECK: // %bb.0:
; CHECK-NEXT: tst w0, #0xffff
; CHECK-NEXT: cset w0, eq
; CHECK-NEXT: ret
%or = or i16 %x, %y
%f = call i16 @llvm.fshr.i16(i16 %or, i16 %x, i16 0)
%r = icmp eq i16 %f, 0
ret i1 %r
}

define i1 @fshr_32_or_eq_0(i16 %x, i16 %y) {
; CHECK-LABEL: fshr_32_or_eq_0:
; CHECK: // %bb.0:
; CHECK-NEXT: tst w0, #0xffff
; CHECK-NEXT: cset w0, eq
; CHECK-NEXT: ret
%or = or i16 %x, %y
%f = call i16 @llvm.fshr.i16(i16 %or, i16 %x, i16 32)
%r = icmp eq i16 %f, 0
ret i1 %r
}