[SDAG] Don't allow implicit trunc in getConstant() #117558
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-llvm-selectiondag Author: Nikita Popov (nikic) ChangesAssert that the passed value is a valid unsigned integer value for the specified type. For signed values getSignedConstant() / getSignedTargetConstant() should be used instead. Full diff: https://github.com/llvm/llvm-project/pull/117558.diff 1 Files Affected:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
index 3a8ec3c6105bc0..f67f30b8c1ee5f 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
@@ -1638,13 +1638,7 @@ SDValue SelectionDAG::getBoolConstant(bool V, const SDLoc &DL, EVT VT,
SDValue SelectionDAG::getConstant(uint64_t Val, const SDLoc &DL, EVT VT,
bool isT, bool isO) {
EVT EltVT = VT.getScalarType();
- assert((EltVT.getSizeInBits() >= 64 ||
- (uint64_t)((int64_t)Val >> EltVT.getSizeInBits()) + 1 < 2) &&
- "getConstant with a uint64_t value that doesn't fit in the type!");
- // TODO: Avoid implicit trunc?
- // See https://github.com/llvm/llvm-project/issues/112510.
- return getConstant(APInt(EltVT.getSizeInBits(), Val, /*isSigned=*/false,
- /*implicitTrunc=*/true),
+ return getConstant(APInt(EltVT.getSizeInBits(), Val, /*isSigned=*/false),
DL, VT, isT, isO);
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Assert that the passed value is a valid unsigned integer value for the specified type. For signed values getSignedConstant() / getSignedTargetConstant() should be used instead.
nikic
force-pushed
the
sdag-implicit-trunc
branch
from
45be4ce to
1195b05
Compare
RKSimon
approved these changes
Nov 25, 2024
| return getConstant(APInt(EltVT.getSizeInBits(), Val, /*isSigned=*/false, | ||
| /*implicitTrunc=*/true), | ||
| DL, VT, isT, isO); | ||
| return getConstant(APInt(EltVT.getSizeInBits(), Val, /*isSigned=*/false), DL, |
There was a problem hiding this comment.
EltVT.getSizeInBits() -> VT.getScalarSizeInBits()
|
Hi @nikic, I am seeing an assertion failure when building the Linux kernel for 32-bit powerpc after this change. char *get_next_block_mtfSymbol;
char get_next_block_mtfSymbol_0;
void get_next_block() {
get_next_block_mtfSymbol_0 = get_next_block_mtfSymbol[-1];
}target datalayout = "E-m:e-p:32:32-Fn32-i64:64-n32"
target triple = "powerpc-unknown-linux-gnu"
define void @get_next_block() {
entry:
%0 = load i8, ptr inttoptr (i32 -1 to ptr), align 1
store i8 %0, ptr null, align 1
ret void
} |
|
@nathanchance I can't reproduce this on current main. Based on the backtrace, I think I already fixed this in 5322415? |
|
@nikic Whoops, looks like the original file got too reduced, how about this one? It reproduces for me at f67ba58. int *get_next_block_base;
char get_next_block_length_0;
void __attribute__get_next_block() {
int i;
short temp[20];
for (i = 0; get_next_block_length_0; i++)
get_next_block_base[i] += temp[i];
}target datalayout = "E-m:e-p:32:32-Fn32-i64:64-n32"
target triple = "powerpc-unknown-linux-gnu"
define void @__attribute__get_next_block() {
entry:
%temp = alloca [20 x i16], i32 0, align 2
br label %for.body
for.body: ; preds = %for.body, %entry
%i.04 = phi i32 [ 0, %entry ], [ %inc, %for.body ]
%arrayidx = getelementptr [20 x i16], ptr %temp, i32 0, i32 %i.04
%0 = load i16, ptr %arrayidx, align 2
%conv = sext i16 %0 to i32
store i32 %conv, ptr null, align 4
%inc = add i32 %i.04, 1
br label %for.body
} |
nikic
added a commit
that referenced
this pull request
Nov 28, 2024
The offset is signed. Fixes assertion failure reported at: #117558 (comment)
|
@nathanchance Thanks, should be fixed by 04a2d50. |
|
@nikic Found another one with struct ufs_clk_scaling {
long long window_start_t;
int enable_attr; int suspend_work;
};
struct ufs_hba {
struct ufs_clk_scaling clk_scaling;
};
void *ufshcd_clk_scaling_suspend_work___mptr;
void ufshcd_clk_scaling_suspend_work() {
struct ufs_hba *hba = ({
ufshcd_clk_scaling_suspend_work___mptr -
__builtin_offsetof(struct ufs_hba, clk_scaling.suspend_work);
});
hba->clk_scaling.window_start_t = 0;
}target datalayout = "e-m:e-p:32:32:32-a:0-n16:32-i64:64:64-i32:32:32-i16:16:16-i1:8:8-f32:32:32-f64:64:64-v32:32:32-v64:64:64-v512:512:512-v1024:1024:1024-v2048:2048:2048"
target triple = "hexagon-unknown-linux-musl"
define void @ufshcd_clk_scaling_suspend_work(ptr %0) {
entry:
%add.ptr = getelementptr i8, ptr %0, i32 -12
store i64 0, ptr %add.ptr, align 8
ret void
}This reproduces for me at 31bde71. |
nikic
added a commit
that referenced
this pull request
Dec 2, 2024
To handle negative offset addrmodes correctly. Fixes #117558 (comment).
|
@nathanchance Thanks, should be fixed by 4851dbb. |
|
Also started to see assertion failures (with big-endian targets) in relation to DAGCombiner::ReduceLoadOpStoreWidth -> getMemBasePlusOffset , when ending up adding negative offsets. I actually consider that as a bug in ReduceLoadOpStoreWidth (that was exposed by this patch) and propose a fix in this pull request: #119203 |
androm3da
added a commit
to androm3da/llvm-project
that referenced
this pull request
Feb 16, 2025
After llvm#117558 landed, this code would assert "Value is not an N-bit unsigned value" in getConstant(), from a test case in zig. Co-authored-by: Craig Topper <craig.topper@sifive.com> Fixes llvm#127296
androm3da
added a commit
to androm3da/llvm-project
that referenced
this pull request
Feb 16, 2025
After llvm#117558 landed, this code would assert "Value is not an N-bit unsigned value" in getConstant(), from a test case in zig. Co-authored-by: Craig Topper <craig.topper@sifive.com> Fixes llvm#127296
androm3da
added a commit
to androm3da/llvm-project
that referenced
this pull request
Feb 16, 2025
After llvm#117558 landed, this code would assert "Value is not an N-bit unsigned value" in getConstant(), from a test case in zig. Co-authored-by: Craig Topper <craig.topper@sifive.com> Fixes llvm#127296
androm3da
added a commit
that referenced
this pull request
Feb 17, 2025
swift-ci
pushed a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 18, 2025
After llvm#117558 landed, this code would assert "Value is not an N-bit unsigned value" in getConstant(), from a test case in zig. Co-authored-by: Craig Topper <craig.topper@sifive.com> Fixes llvm#127296 (cherry picked from commit 788cb72)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Assert that the passed value is a valid unsigned integer value for the specified type.
For signed values getSignedConstant() / getSignedTargetConstant() should be used instead.