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 all commits
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
14 changes: 7 additions & 7 deletions llvm/lib/Target/LoongArch/LoongArchISelDAGToDAG.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ void LoongArchDAGToDAGISel::Select(SDNode *Node) {
Result = CurDAG->getMachineNode(
Inst.Opc, DL, GRLenVT,
{SrcReg, SrcReg,
CurDAG->getTargetConstant(Inst.Imm >> 32, DL, GRLenVT),
CurDAG->getSignedTargetConstant(Inst.Imm >> 32, DL, GRLenVT),
CurDAG->getTargetConstant(Inst.Imm & 0xFF, DL, GRLenVT)});
break;
default:
Expand Down Expand Up @@ -233,7 +233,7 @@ bool LoongArchDAGToDAGISel::SelectAddrConstant(SDValue Addr, SDValue &Base,
if (!isInt<12>(CVal))
return false;
Base = CurDAG->getRegister(LoongArch::R0, VT);
Offset = CurDAG->getTargetConstant(SignExtend64<12>(CVal), DL, VT);
Offset = CurDAG->getSignedTargetConstant(SignExtend64<12>(CVal), DL, VT);
return true;
}

Expand All @@ -255,7 +255,7 @@ bool LoongArchDAGToDAGISel::SelectAddrRegImm12(SDValue Addr, SDValue &Base,
int64_t Imm = cast<ConstantSDNode>(Addr.getOperand(1))->getSExtValue();
if (isInt<12>(Imm)) {
Base = Addr.getOperand(0);
Offset = CurDAG->getTargetConstant(SignExtend64<12>(Imm), DL, VT);
Offset = CurDAG->getSignedTargetConstant(SignExtend64<12>(Imm), DL, VT);
return true;
}
}
Expand Down Expand Up @@ -398,8 +398,8 @@ bool LoongArchDAGToDAGISel::selectVSplatImm(SDValue N, SDValue &SplatVal) {
if (selectVSplat(N.getNode(), ImmValue, EltTy.getSizeInBits()) &&
ImmValue.getBitWidth() == EltTy.getSizeInBits()) {
if (IsSigned && ImmValue.isSignedIntN(ImmBitSize)) {
SplatVal = CurDAG->getTargetConstant(ImmValue.getSExtValue(), SDLoc(N),
Subtarget->getGRLenVT());
SplatVal = CurDAG->getSignedTargetConstant(
ImmValue.getSExtValue(), SDLoc(N), Subtarget->getGRLenVT());
return true;
}
if (!IsSigned && ImmValue.isIntN(ImmBitSize)) {
Expand All @@ -425,7 +425,7 @@ bool LoongArchDAGToDAGISel::selectVSplatUimmInvPow2(SDValue N,
int32_t Log2 = (~ImmValue).exactLogBase2();

if (Log2 != -1) {
SplatImm = CurDAG->getTargetConstant(Log2, SDLoc(N), EltTy);
SplatImm = CurDAG->getSignedTargetConstant(Log2, SDLoc(N), EltTy);
return true;
}
}
Expand All @@ -446,7 +446,7 @@ bool LoongArchDAGToDAGISel::selectVSplatUimmPow2(SDValue N,
int32_t Log2 = ImmValue.exactLogBase2();

if (Log2 != -1) {
SplatImm = CurDAG->getTargetConstant(Log2, SDLoc(N), EltTy);
SplatImm = CurDAG->getSignedTargetConstant(Log2, SDLoc(N), EltTy);
return true;
}
}
Expand Down
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: 6 additions & 6 deletions llvm/lib/Target/LoongArch/LoongArchInstrInfo.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,22 +524,22 @@ def ImmSubFrom32 : SDNodeXForm<imm, [{

// Return the lowest 12 bits of the signed immediate.
def LO12: SDNodeXForm<imm, [{
return CurDAG->getTargetConstant(SignExtend64<12>(N->getSExtValue()),
SDLoc(N), N->getValueType(0));
return CurDAG->getSignedTargetConstant(SignExtend64<12>(N->getSExtValue()),
SDLoc(N), N->getValueType(0));
}]>;

// Return the higher 16 bits of the signed immediate.
def HI16 : SDNodeXForm<imm, [{
return CurDAG->getTargetConstant(N->getSExtValue() >> 16, SDLoc(N),
N->getValueType(0));
return CurDAG->getSignedTargetConstant(N->getSExtValue() >> 16, SDLoc(N),
N->getValueType(0));
}]>;

// Return the higher 16 bits of the signed immediate, adjusted for use within an
// `addu16i.d + addi` pair.
def HI16ForAddu16idAddiPair: SDNodeXForm<imm, [{
auto Imm = N->getSExtValue();
return CurDAG->getTargetConstant((Imm - SignExtend64<12>(Imm)) >> 16,
SDLoc(N), N->getValueType(0));
return CurDAG->getSignedTargetConstant((Imm - SignExtend64<12>(Imm)) >> 16,
SDLoc(N), N->getValueType(0));
}]>;

def BaseAddr : ComplexPattern<iPTR, 1, "SelectBaseAddr">;
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/LoongArch/LoongArchLSXInstrInfo.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,8 @@ def lsxsplatf64 : PatFrag<(ops node:$e0),

def to_valid_timm : SDNodeXForm<timm, [{
auto CN = cast<ConstantSDNode>(N);
return CurDAG->getTargetConstant(CN->getSExtValue(), SDLoc(N), Subtarget->getGRLenVT());
return CurDAG->getSignedTargetConstant(CN->getSExtValue(), SDLoc(N),
Subtarget->getGRLenVT());
}]>;

// FP immediate of VLDI patterns.
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