Skip to content

[LoongArch] Fix out-of-range assert in DAG constant getting #141586

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 28, 2025
Merged

[LoongArch] Fix out-of-range assert in DAG constant getting #141586

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
00944f9
[LoongArch] Fix assertion failure in performORCombine
heiher May 27, 2025
71617ca
Add defensive fixes
heiher May 27, 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
2 changes: 1 addition & 1 deletion llvm/lib/Target/LoongArch/LoongArchISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4410,7 +4410,7 @@ static SDValue performORCombine(SDNode *N, SelectionDAG &DAG,
LLVM_DEBUG(dbgs() << "Perform OR combine: match pattern 5\n");
return DAG.getNode(
LoongArchISD::BSTRINS, DL, ValTy, N0.getOperand(0),
DAG.getConstant(CN1->getSExtValue() >> MaskIdx0, DL, ValTy),
DAG.getSignedConstant(CN1->getSExtValue() >> MaskIdx0, DL, ValTy),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do other calls to DAG.getConstant() have the same issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps. I haven't found any others so far, but if I do, I might fix them in this PR or in a follow-up.

Copy link
Member Author

Choose a reason for hiding this comment

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

I came across a few more spots that might be risky, based on the data types and value ranges of getConstant() and getTargetConstant(). I’ll include defensive fixes in this PR, but given the low risk and similarity of the cases, I’m not planning to add separate tests for each one.

DAG.getConstant(ValBits == 32 ? (MaskIdx0 + (MaskLen0 & 31) - 1)
: (MaskIdx0 + MaskLen0 - 1),
DL, GRLenVT),
Expand Down
12 changes: 12 additions & 0 deletions llvm/test/CodeGen/LoongArch/bstrins_w.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,18 @@ define i32 @pat8(i32 %c) nounwind {
ret i32 %or
}

define i32 @pat9(i32 %a) {
; CHECK-LABEL: pat9:
; CHECK: # %bb.0:
; CHECK-NEXT: lu12i.w $a1, -8
; CHECK-NEXT: ori $a1, $a1, 564
; CHECK-NEXT: bstrins.w $a0, $a1, 31, 16
; CHECK-NEXT: ret
%and = and i32 %a, 65535 ; 0x0000ffff
%or = or i32 %and, -2110521344 ; 0x82340000
ret i32 %or
}

;; Test that bstrins.w is not generated because constant OR operand
;; doesn't fit into bits cleared by constant AND operand.
define i32 @no_bstrins_w(i32 %a) nounwind {
Expand Down