Verifier: forbid non-i32/i64 lrint, and non-i64 llrint #70839
Conversation
The LangRef clearly specifies that the return type of llvm.lrint must be i32, i64, or a vector thereof, and that the return type of llvm.llrint must be i64, or a vector thereof. Check this in the IR Verifier, and bail out early if this is not the case. Furthermore, update a couple of tests that were mistakenly using a non-existent i32-variant of llvm.llrint.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-ir Author: Ramkumar Ramachandra (artagnon) ChangesThe LangRef clearly specifies that the return type of llvm.lrint must be i32, i64, or a vector thereof, and that the return type of llvm.llrint must be i64, or a vector thereof. Check this in the IR Verifier, and bail out early if this is not the case. Furthermore, update a couple of tests that were mistakenly using a non-existent i32-variant of llvm.llrint. Full diff: https://github.com/llvm/llvm-project/pull/70839.diff 3 Files Affected:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 396af600b8dab29..81384befef5b947 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -5673,11 +5673,21 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
case Intrinsic::llrint: {
Type *ValTy = Call.getArgOperand(0)->getType();
Type *ResultTy = Call.getType();
+ unsigned ResultSz = ResultTy->getScalarSizeInBits();
Check(
ValTy->isFPOrFPVectorTy() && ResultTy->isIntOrIntVectorTy(),
"llvm.lrint, llvm.llrint: argument must be floating-point or vector "
"of floating-points, and result must be integer or vector of integers",
&Call);
+
+ if (ID == Intrinsic::lrint)
+ Check(ResultSz == 32 || ResultSz == 64,
+ "llvm.lrint: result type must be i32, i64, or a vector thereof",
+ &Call);
+ else
+ Check(ResultSz == 64,
+ "llvm.llrint: result type must be i64 or a vector thereof", &Call);
+
Check(ValTy->isVectorTy() == ResultTy->isVectorTy(),
"llvm.lrint, llvm.llrint: argument and result disagree on vector use",
&Call);
diff --git a/llvm/test/Transforms/InstCombine/freeze-fp-ops.ll b/llvm/test/Transforms/InstCombine/freeze-fp-ops.ll
index 9fb69015a8f7dca..3f147539e110cf3 100644
--- a/llvm/test/Transforms/InstCombine/freeze-fp-ops.ll
+++ b/llvm/test/Transforms/InstCombine/freeze-fp-ops.ll
@@ -411,15 +411,15 @@ define i32 @freeze_lrint(float %arg) {
ret i32 %freeze
}
-define i32 @freeze_llrint(float %arg) {
+define i64 @freeze_llrint(float %arg) {
; CHECK-LABEL: @freeze_llrint(
; CHECK-NEXT: [[ARG_FR:%.*]] = freeze float [[ARG:%.*]]
-; CHECK-NEXT: [[OP:%.*]] = call i32 @llvm.llrint.i32.f32(float [[ARG_FR]])
-; CHECK-NEXT: ret i32 [[OP]]
+; CHECK-NEXT: [[OP:%.*]] = call i64 @llvm.llrint.i64.f32(float [[ARG_FR]])
+; CHECK-NEXT: ret i64 [[OP]]
;
- %op = call i32 @llvm.llrint.i32.f32(float %arg)
- %freeze = freeze i32 %op
- ret i32 %freeze
+ %op = call i64 @llvm.llrint.i64.f32(float %arg)
+ %freeze = freeze i64 %op
+ ret i64 %freeze
}
define i32 @freeze_noundef_lround(float %arg) {
@@ -452,14 +452,14 @@ define i32 @freeze_noundef_lrint(float %arg) {
ret i32 %freeze
}
-define i32 @freeze_noundef_llrint(float %arg) {
+define i64 @freeze_noundef_llrint(float %arg) {
; CHECK-LABEL: @freeze_noundef_llrint(
-; CHECK-NEXT: [[OP:%.*]] = call noundef i32 @llvm.llrint.i32.f32(float [[ARG:%.*]])
-; CHECK-NEXT: ret i32 [[OP]]
+; CHECK-NEXT: [[OP:%.*]] = call noundef i64 @llvm.llrint.i64.f32(float [[ARG:%.*]])
+; CHECK-NEXT: ret i64 [[OP]]
;
- %op = call noundef i32 @llvm.llrint.i32.f32(float %arg)
- %freeze = freeze i32 %op
- ret i32 %freeze
+ %op = call noundef i64 @llvm.llrint.i64.f32(float %arg)
+ %freeze = freeze i64 %op
+ ret i64 %freeze
}
define float @freeze_minnum(float %arg0, float noundef %arg1) {
@@ -603,7 +603,7 @@ declare float @llvm.arithmetic.fence.f32(float)
declare i32 @llvm.lround.i32.f32(float)
declare i32 @llvm.llround.i32.f32(float)
declare i32 @llvm.lrint.i32.f32(float)
-declare i32 @llvm.llrint.i32.f32(float)
+declare i64 @llvm.llrint.i64.f32(float)
declare float @llvm.minnum.f32(float, float)
declare float @llvm.maxnum.f32(float, float)
declare float @llvm.minimum.f32(float, float)
diff --git a/llvm/test/Transforms/Scalarizer/intrinsics.ll b/llvm/test/Transforms/Scalarizer/intrinsics.ll
index f5e5be1f5998e00..2037ea4a2e794d3 100644
--- a/llvm/test/Transforms/Scalarizer/intrinsics.ll
+++ b/llvm/test/Transforms/Scalarizer/intrinsics.ll
@@ -31,7 +31,7 @@ declare <2 x i32> @llvm.fptoui.sat.v2i32.v2f32(<2 x float>)
; Unary fp operand, int return type
declare <2 x i32> @llvm.lrint.v2i32.v2f32(<2 x float>)
-declare <2 x i32> @llvm.llrint.v2i32.v2f32(<2 x float>)
+declare <2 x i64> @llvm.llrint.v2i64.v2f32(<2 x float>)
; Bool return type, overloaded on fp operand type
declare <2 x i1> @llvm.is.fpclass(<2 x float>, i32)
@@ -224,13 +224,13 @@ define <2 x i32> @scalarize_lrint(<2 x float> %x) #0 {
ret <2 x i32> %rnd
}
-define <2 x i32> @scalarize_llrint(<2 x float> %x) #0 {
+define <2 x i64> @scalarize_llrint(<2 x float> %x) #0 {
; CHECK-LABEL: @scalarize_llrint(
-; CHECK-NEXT: [[RND:%.*]] = call <2 x i32> @llvm.llrint.v2i32.v2f32(<2 x float> [[X:%.*]])
-; CHECK-NEXT: ret <2 x i32> [[RND]]
+; CHECK-NEXT: [[RND:%.*]] = call <2 x i64> @llvm.llrint.v2i64.v2f32(<2 x float> [[X:%.*]])
+; CHECK-NEXT: ret <2 x i64> [[RND]]
;
- %rnd = call <2 x i32> @llvm.llrint.v2i32.v2f32(<2 x float> %x)
- ret <2 x i32> %rnd
+ %rnd = call <2 x i64> @llvm.llrint.v2i64.v2f32(<2 x float> %x)
+ ret <2 x i64> %rnd
}
define <2 x i1> @scalarize_is_fpclass(<2 x float> %x) #0 {
|
|
The examples in the LangRef are not usally an exhaustive list of all types supported. Does the text description say only i32 and i64 are supported? |
|
Oh. No, the text description doesn't say so explicitly, but we normally lower llvm.lrint and llvm.llrint to libcalls: how is the libcall to lrint supposed to return anything but i32/i64, and that to llrint supposed to return anything but i64? I suppose we could widen/narrow the type in ISelLowering after getting the result of the libcall, but afaik, we do no such thing. Currently, a non-i32/i64 [l]lrint asserts while lowering on many architectures I tested. I suppose the commit message requires rewording. |
|
Moreover, as evident from the patch, there is no test coverage for non-i32/i64 llvm.lrint and non-i64 llvm.llrint in test/CodeGen for any target. If future contributors want to support these things, they can remove the Verifier check, and add test coverage along with their patch. As it currently stands, I'm convinced that we don't support these configurations in practice, even if the LangRef isn't explicit about this. |
There's sort of an implicit contract that the scalar intrinsics would only be created by calls to the functions in C code so the types match the C type for long or long long which LLVM itself doesn't know about. If the target doesn't lower them to instructions we map them back to the C library call. That's why we maintain separate intrinsics for lrint and llrint so we can use the name to get back to the correct library call without LLVM knowing the size of long or long long. Theoretically an out of tree target could have a 128 bit |
I see: that makes sense, and I will close this PR. However, I don't quite understand the ISelLowering for different targets: when the LangRef says "not all targets may support all types, however", is the user expected to experience random crashes from SelectionDAG (these are pretty nasty) when they try to lower some unsupported type + call + target combination? |
In practice, the return type of llvm.lrint must be i32, i64, or a vector thereof, and that the return type of llvm.llrint must be i64, or a vector thereof, even if the LangRef isn't explicit about this. This is because we often lower llvm.lrint and llvm.llrint to a libcall, and make no attempt to widen/narrow the result. Check this in the IR Verifier, and bail out early if this is not the case. As evidenced by this patch, there are no CodeGen tests written for any target that support the forbidden configurations. Furthermore, update a couple of Transform tests that were mistakenly using a non-existent i32-variant of llvm.llrint.