[CIR] Correctly emit the size expr of a VLA with a 'bool' size. - #200066
Conversation
This came up in a test suite, the frontend considers 'bool' to be
integral enough to use as a bounds, so bool makes it through to the VLA
codegen. This patch adds a new cast function that takes a bool or int
type and casts it to an int (as this is a pretty useful task, that I
believe I've seen before).
|
@llvm/pr-subscribers-clangir Author: Erich Keane (erichkeane) ChangesThis came up in a test suite, the frontend considers 'bool' to be integral enough to use as a bounds, so bool makes it through to the VLA codegen. This patch adds a new cast function that takes a bool or int type and casts it to an int (as this is a pretty useful task, that I Full diff: https://github.com/llvm/llvm-project/pull/200066.diff 3 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
index f5222accff154..f8bf946301000 100644
--- a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
+++ b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
@@ -513,6 +513,15 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
return createCast(src.getLoc(), kind, src, newTy);
}
+ // Creates a cast from bool or int to an integer type.
+ mlir::Value createBoolIntToIntCast(mlir::Value src, mlir::Type newTy) {
+ if (newTy == src.getType())
+ return src;
+ if (src.getType() == getBoolTy())
+ return createBoolToInt(src, newTy);
+ return createIntCast(src, newTy);
+ }
+
mlir::Value createIntCast(mlir::Value src, mlir::Type newTy) {
return createCast(cir::CastKind::integral, src, newTy);
}
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
index 5aa203d444a79..dd83f9205fbf9 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -1653,10 +1653,12 @@ void CIRGenFunction::emitVariablyModifiedType(QualType type) {
mlir::Value size = emitScalarExpr(sizeExpr);
assert(!cir::MissingFeatures::sanitizers());
+ size.getType().dump();
+
// Always zexting here would be wrong if it weren't
// undefined behavior to have a negative bound.
// FIXME: What about when size's type is larger than size_t?
- entry = builder.createIntCast(size, sizeTy);
+ entry = builder.createBoolIntToIntCast(size, sizeTy);
}
}
type = vat->getElementType();
diff --git a/clang/test/CIR/CodeGen/vla.c b/clang/test/CIR/CodeGen/vla.c
index f86ca88bc124c..c205808ec78fa 100644
--- a/clang/test/CIR/CodeGen/vla.c
+++ b/clang/test/CIR/CodeGen/vla.c
@@ -49,6 +49,47 @@ void f0(int len) {
// OGCG: %[[STACK_RESTORE_PTR:.*]] = load ptr, ptr %[[SAVED_STACK]]
// OGCG: call void @llvm.stackrestore.p0(ptr %[[STACK_RESTORE_PTR]])
+void vla_bool_size(_Bool len) {
+ int arr[len];
+}
+
+// CIR-LABEL: cir.func {{.*}}@vla_bool_size
+// CIR: %[[LEN_ADDR:.*]] = cir.alloca !cir.bool, !cir.ptr<!cir.bool>, ["len", init]
+// CIR: %[[SAVED_STACK:.*]] = cir.alloca !cir.ptr<!u8i>, !cir.ptr<!cir.ptr<!u8i>>, ["saved_stack"]
+// CIR: %[[LEN:.*]] = cir.load{{.*}} %[[LEN_ADDR]]
+// CIR: %[[LEN_SIZE_T:.*]] = cir.cast bool_to_int %[[LEN]] : !cir.bool -> !u64i
+// CIR: %[[STACK_PTR:.*]] = cir.stacksave
+// CIR: cir.store{{.*}} %[[STACK_PTR]], %[[SAVED_STACK]]
+// CIR: %[[ARR:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, %[[LEN_SIZE_T]] : !u64i, ["arr"]
+// CIR: %[[STACK_RESTORE_PTR:.*]] = cir.load{{.*}} %[[SAVED_STACK]]
+// CIR: cir.stackrestore %[[STACK_RESTORE_PTR]]
+//
+// LLVM-LABEL: define {{.*}}@vla_bool_size
+// LLVM: %[[LEN_ADDR:.*]] = alloca i8
+// LLVM: %[[SAVED_STACK:.*]] = alloca ptr
+// LLVM: %[[LEN:.*]] = load i8, ptr %[[LEN_ADDR]]
+// LLVM: %[[LEN_TRUNC:.*]] = trunc i8 %[[LEN]] to i1
+// LLVM: %[[LEN_SIZE_T:.*]] = zext i1 %[[LEN_TRUNC]] to i64
+// LLVM: %[[STACK_PTR:.*]] = call ptr @llvm.stacksave.p0()
+// LLVM: store ptr %[[STACK_PTR]], ptr %[[SAVED_STACK]]
+// LLVM: %[[ARR:.*]] = alloca i32, i64 %[[LEN_SIZE_T]]
+// LLVM: %[[STACK_RESTORE_PTR:.*]] = load ptr, ptr %[[SAVED_STACK]]
+// LLVM: call void @llvm.stackrestore.p0(ptr %[[STACK_RESTORE_PTR]])
+
+// OGCG-LABEL: define {{.*}}@vla_bool_size
+// OGCG: %[[LEN_ADDR:.*]] = alloca i8
+// OGCG: %[[SAVED_STACK:.*]] = alloca ptr
+// OGCG: %[[VLA_EXPR0:.*]] = alloca i64
+// OGCG: %[[LEN:.*]] = load i8, ptr %[[LEN_ADDR]]
+// OGCG: %[[LEN_CMP:.*]] = icmp ne i8 %[[LEN]]
+// OGCG: %[[LEN_SIZE_T:.*]] = zext i1 %[[LEN_CMP]] to i64
+// OGCG: %[[STACK_PTR:.*]] = call ptr @llvm.stacksave.p0()
+// OGCG: store ptr %[[STACK_PTR]], ptr %[[SAVED_STACK]]
+// OGCG: %[[ARR:.*]] = alloca i32, i64 %[[LEN_SIZE_T]]
+// OGCG: store i64 %[[LEN_SIZE_T]], ptr %[[VLA_EXPR0]]
+// OGCG: %[[STACK_RESTORE_PTR:.*]] = load ptr, ptr %[[SAVED_STACK]]
+// OGCG: call void @llvm.stackrestore.p0(ptr %[[STACK_RESTORE_PTR]])
+
void f1(int len) {
int arr[16][len];
}
|
|
@llvm/pr-subscribers-clang Author: Erich Keane (erichkeane) ChangesThis came up in a test suite, the frontend considers 'bool' to be integral enough to use as a bounds, so bool makes it through to the VLA codegen. This patch adds a new cast function that takes a bool or int type and casts it to an int (as this is a pretty useful task, that I Full diff: https://github.com/llvm/llvm-project/pull/200066.diff 3 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
index f5222accff154..f8bf946301000 100644
--- a/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
+++ b/clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h
@@ -513,6 +513,15 @@ class CIRBaseBuilderTy : public mlir::OpBuilder {
return createCast(src.getLoc(), kind, src, newTy);
}
+ // Creates a cast from bool or int to an integer type.
+ mlir::Value createBoolIntToIntCast(mlir::Value src, mlir::Type newTy) {
+ if (newTy == src.getType())
+ return src;
+ if (src.getType() == getBoolTy())
+ return createBoolToInt(src, newTy);
+ return createIntCast(src, newTy);
+ }
+
mlir::Value createIntCast(mlir::Value src, mlir::Type newTy) {
return createCast(cir::CastKind::integral, src, newTy);
}
diff --git a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
index 5aa203d444a79..dd83f9205fbf9 100644
--- a/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenFunction.cpp
@@ -1653,10 +1653,12 @@ void CIRGenFunction::emitVariablyModifiedType(QualType type) {
mlir::Value size = emitScalarExpr(sizeExpr);
assert(!cir::MissingFeatures::sanitizers());
+ size.getType().dump();
+
// Always zexting here would be wrong if it weren't
// undefined behavior to have a negative bound.
// FIXME: What about when size's type is larger than size_t?
- entry = builder.createIntCast(size, sizeTy);
+ entry = builder.createBoolIntToIntCast(size, sizeTy);
}
}
type = vat->getElementType();
diff --git a/clang/test/CIR/CodeGen/vla.c b/clang/test/CIR/CodeGen/vla.c
index f86ca88bc124c..c205808ec78fa 100644
--- a/clang/test/CIR/CodeGen/vla.c
+++ b/clang/test/CIR/CodeGen/vla.c
@@ -49,6 +49,47 @@ void f0(int len) {
// OGCG: %[[STACK_RESTORE_PTR:.*]] = load ptr, ptr %[[SAVED_STACK]]
// OGCG: call void @llvm.stackrestore.p0(ptr %[[STACK_RESTORE_PTR]])
+void vla_bool_size(_Bool len) {
+ int arr[len];
+}
+
+// CIR-LABEL: cir.func {{.*}}@vla_bool_size
+// CIR: %[[LEN_ADDR:.*]] = cir.alloca !cir.bool, !cir.ptr<!cir.bool>, ["len", init]
+// CIR: %[[SAVED_STACK:.*]] = cir.alloca !cir.ptr<!u8i>, !cir.ptr<!cir.ptr<!u8i>>, ["saved_stack"]
+// CIR: %[[LEN:.*]] = cir.load{{.*}} %[[LEN_ADDR]]
+// CIR: %[[LEN_SIZE_T:.*]] = cir.cast bool_to_int %[[LEN]] : !cir.bool -> !u64i
+// CIR: %[[STACK_PTR:.*]] = cir.stacksave
+// CIR: cir.store{{.*}} %[[STACK_PTR]], %[[SAVED_STACK]]
+// CIR: %[[ARR:.*]] = cir.alloca !s32i, !cir.ptr<!s32i>, %[[LEN_SIZE_T]] : !u64i, ["arr"]
+// CIR: %[[STACK_RESTORE_PTR:.*]] = cir.load{{.*}} %[[SAVED_STACK]]
+// CIR: cir.stackrestore %[[STACK_RESTORE_PTR]]
+//
+// LLVM-LABEL: define {{.*}}@vla_bool_size
+// LLVM: %[[LEN_ADDR:.*]] = alloca i8
+// LLVM: %[[SAVED_STACK:.*]] = alloca ptr
+// LLVM: %[[LEN:.*]] = load i8, ptr %[[LEN_ADDR]]
+// LLVM: %[[LEN_TRUNC:.*]] = trunc i8 %[[LEN]] to i1
+// LLVM: %[[LEN_SIZE_T:.*]] = zext i1 %[[LEN_TRUNC]] to i64
+// LLVM: %[[STACK_PTR:.*]] = call ptr @llvm.stacksave.p0()
+// LLVM: store ptr %[[STACK_PTR]], ptr %[[SAVED_STACK]]
+// LLVM: %[[ARR:.*]] = alloca i32, i64 %[[LEN_SIZE_T]]
+// LLVM: %[[STACK_RESTORE_PTR:.*]] = load ptr, ptr %[[SAVED_STACK]]
+// LLVM: call void @llvm.stackrestore.p0(ptr %[[STACK_RESTORE_PTR]])
+
+// OGCG-LABEL: define {{.*}}@vla_bool_size
+// OGCG: %[[LEN_ADDR:.*]] = alloca i8
+// OGCG: %[[SAVED_STACK:.*]] = alloca ptr
+// OGCG: %[[VLA_EXPR0:.*]] = alloca i64
+// OGCG: %[[LEN:.*]] = load i8, ptr %[[LEN_ADDR]]
+// OGCG: %[[LEN_CMP:.*]] = icmp ne i8 %[[LEN]]
+// OGCG: %[[LEN_SIZE_T:.*]] = zext i1 %[[LEN_CMP]] to i64
+// OGCG: %[[STACK_PTR:.*]] = call ptr @llvm.stacksave.p0()
+// OGCG: store ptr %[[STACK_PTR]], ptr %[[SAVED_STACK]]
+// OGCG: %[[ARR:.*]] = alloca i32, i64 %[[LEN_SIZE_T]]
+// OGCG: store i64 %[[LEN_SIZE_T]], ptr %[[VLA_EXPR0]]
+// OGCG: %[[STACK_RESTORE_PTR:.*]] = load ptr, ptr %[[SAVED_STACK]]
+// OGCG: call void @llvm.stackrestore.p0(ptr %[[STACK_RESTORE_PTR]])
+
void f1(int len) {
int arr[16][len];
}
|
andykaylor
left a comment
There was a problem hiding this comment.
lgtm except for the debugging artifact.
| mlir::Value size = emitScalarExpr(sizeExpr); | ||
| assert(!cir::MissingFeatures::sanitizers()); | ||
|
|
||
| size.getType().dump(); |
There was a problem hiding this comment.
This is debugging code, right?
| // OGCG: %[[SAVED_STACK:.*]] = alloca ptr | ||
| // OGCG: %[[VLA_EXPR0:.*]] = alloca i64 | ||
| // OGCG: %[[LEN:.*]] = load i8, ptr %[[LEN_ADDR]] | ||
| // OGCG: %[[LEN_CMP:.*]] = icmp ne i8 %[[LEN]] |
There was a problem hiding this comment.
Unrelated to your change, but this is an interesting difference from CIR. We're storing a zero-extended i1 as an i8, then loading it as an i8, so the truncation performed by CIR in this case should be strictly safe. I'm just wondering if there could be any circumstances where we'd need to compare to zero. I guess it depends on what expression led to the truncation?
There was a problem hiding this comment.
IIRC: The standards require that the LSB be 1 for a bool now, so our version is safe. Otherwise it is UB, since the rest of the bool is padding, their values are irrelevant (and shouldn't be compared!). So the cmp and the trunc are equivalent I believe.
This came up in a test suite, the frontend considers 'bool' to be integral enough to use as a bounds, so bool makes it through to the VLA codegen. This patch adds a new cast function that takes a bool or int type and casts it to an int (as this is a pretty useful task, that I
believe I've seen before).