-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[CIR] Upstream handling of integral-to-pointer casts #161653
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.
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
Conversation
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-clang Author: Shawn K (kimsh02) ChangesFix #153658 Handle integral to pointer casts and port the relevant Full diff: https://github.com/llvm/llvm-project/pull/161653.diff 3 Files Affected:
diff --git a/clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h b/clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h
index 417a226e44cbf..c7450d8770714 100644
--- a/clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h
+++ b/clang/include/clang/CIR/Dialect/IR/CIRDataLayout.h
@@ -14,6 +14,11 @@
#include "mlir/Dialect/DLTI/DLTI.h"
#include "mlir/IR/BuiltinOps.h"
+#include "clang/CIR/Dialect/IR/CIRAttrs.h"
+#include "clang/CIR/Dialect/IR/CIRTypes.h"
+#include "llvm/IR/DataLayout.h"
+#include "llvm/Support/Alignment.h"
+#include "llvm/Support/TypeSize.h"
namespace cir {
@@ -81,6 +86,19 @@ class CIRDataLayout {
}
llvm::TypeSize getTypeSizeInBits(mlir::Type ty) const;
+
+ llvm::TypeSize getPointerTypeSizeInBits(mlir::Type Ty) const {
+ assert(mlir::isa<cir::PointerType>(Ty) &&
+ "This should only be called with a pointer type");
+ return layout.getTypeSizeInBits(Ty);
+ }
+
+ mlir::Type getIntPtrType(mlir::Type Ty) const {
+ assert(mlir::isa<cir::PointerType>(Ty) && "Expected pointer type");
+ auto IntTy =
+ cir::IntType::get(Ty.getContext(), getPointerTypeSizeInBits(Ty), false);
+ return IntTy;
+ }
};
} // namespace cir
diff --git a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
index 500007f6f241b..c64c5f7d7a9c8 100644
--- a/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp
@@ -1889,6 +1889,25 @@ mlir::Value ScalarExprEmitter::VisitCastExpr(CastExpr *ce) {
}
return v;
}
+ case CK_IntegralToPointer: {
+ auto DestCIRTy = cgf.convertType(destTy);
+ mlir::Value Src = Visit(const_cast<Expr *>(subExpr));
+
+ // Properly resize by casting to an int of the same size as the pointer.
+ // Clang's IntegralToPointer includes 'bool' as the source, but in CIR
+ // 'bool' is not an integral type. So check the source type to get the
+ // correct CIR conversion.
+ auto MiddleTy = cgf.cgm.getDataLayout().getIntPtrType(DestCIRTy);
+ auto MiddleVal = builder.createCast(subExpr->getType()->isBooleanType()
+ ? cir::CastKind::bool_to_int
+ : cir::CastKind::integral,
+ Src, MiddleTy);
+
+ if (cgf.cgm.getCodeGenOpts().StrictVTablePointers)
+ llvm_unreachable("NYI");
+
+ return builder.createIntToPtr(MiddleVal, DestCIRTy);
+ }
case CK_ArrayToPointerDecay:
return cgf.emitArrayToPointerDecay(subExpr).getPointer();
diff --git a/clang/test/CIR/CodeGen/cast.cpp b/clang/test/CIR/CodeGen/cast.cpp
index 7afa955cf3bcf..a1fd92612fe37 100644
--- a/clang/test/CIR/CodeGen/cast.cpp
+++ b/clang/test/CIR/CodeGen/cast.cpp
@@ -131,3 +131,25 @@ void bitcast() {
// LLVM: %[[D_VEC:.*]] = load <2 x double>, ptr {{.*}}, align 16
// LLVM: %[[I_VEC:.*]] = bitcast <2 x double> %[[D_VEC]] to <4 x i32>
+
+void f(long int start) {
+ void *p = (void*)start;
+}
+// CIR: %[[L:.*]] = cir.load {{.*}} : !cir.ptr<!s64i>, !s64i
+// CIR: %[[MID:.*]] = cir.cast integral %[[L]] : !s64i -> !u64i
+// CIR: cir.cast int_to_ptr %[[MID]] : !u64i -> !cir.ptr<!void>
+
+struct A { int x; };
+
+void int_cast(long ptr) {
+ ((A *)ptr)->x = 0;
+}
+// CIR: cir.cast int_to_ptr {{.*}} : !u64i -> !cir.ptr<!rec_A>
+// LLVM: inttoptr {{.*}} to ptr
+
+void null_cast(long) {
+ *(int *)0 = 0;
+ ((A *)0)->x = 0;
+}
+// CIR: #cir.ptr<null> : !cir.ptr<!s32i>
+// CIR: #cir.ptr<null> : !cir.ptr<!rec_A>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
What you've done here is basically correct. I have a lot of comments, but they're all just a matter of adapting to the expectations for upstreaming CIR code. I hope you won't be discouraged.
clang/test/CIR/CodeGen/cast.cpp
Outdated
// CIR: #cir.ptr<null> : !cir.ptr<!s32i> | ||
// CIR: #cir.ptr<null> : !cir.ptr<!rec_A> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These checks need to be expanded. The CIR generated should look like this:
%1 = cir.const #cir.int<0> : !s32i
%2 = cir.const #cir.ptr<null> : !cir.ptr<!s32i>
cir.store align(4) %1, %2 : !s32i, !cir.ptr<!s32i>
%3 = cir.const #cir.int<0> : !s32i
%4 = cir.const #cir.ptr<null> : !cir.ptr<!rec_A>
%5 = cir.get_member %4[0] {name = "x"} : !cir.ptr<!rec_A> -> !cir.ptr<!s32i>
cir.store align(4) %3, %5 : !s32i, !cir.ptr<!s32i>
The point of the test is to verify that we're using a null pointer in the expected instructions, so I'd check at least that (and always use regex substitutions for SSA values):
// CIR: %[[NULLPTR:.*]] = cir.const #cir.ptr<null> : !cir.ptr<!s32i>
// CIR: cir.store{{.*}} %{{.*}}, %[[NULLPTR]] : !s32i, !cir.ptr<!s32i>
// CIR: %[[NULLPTR_A:.*]] = cir.const #cir.ptr<null> : !cir.ptr<!rec_A>
// CIR: %[[A_X:.*]] = cir.get_member %[[NULLPTR_A]][0] {name = "x"} : !cir.ptr<!rec_A> -> !cir.ptr<!s32i>
Note that I've used %{{.*}}
for the value being stored in the second line, because we don't need to test that here. Also note, I used {{.*}}
following cir.store
so that this test will pass with or without an alignment specifier, because that's not relevant to this test and we wouldn't want to have to update this test if alignment behavior is changed in the future.
Nothing specific to casts is being lowered to LLVM IR, so I don't think LLVM checks are needed here.
Co-authored-by: Andy Kaylor <[email protected]>
Co-authored-by: Andy Kaylor <[email protected]>
Co-authored-by: Andy Kaylor <[email protected]>
Co-authored-by: Andy Kaylor <[email protected]>
Co-authored-by: Andy Kaylor <[email protected]>
Thanks for all the helpful feedback! I've applied the suggested commits, and I'll revisit the headers and IR checks. |
My suggested changes didn't include all uses of the variables that were renamed, so you'll need to do that manually, as well as fixing formatting issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @kimsh02 for the PR!
I added some small suggestions
Co-authored-by: Amr Hesham <[email protected]>
Co-authored-by: Amr Hesham <[email protected]>
Co-authored-by: Amr Hesham <[email protected]>
Co-authored-by: Amr Hesham <[email protected]>
Co-authored-by: Amr Hesham <[email protected]>
Thanks, @AmrDeveloper ! I've applied the suggested commits. |
@andykaylor Yes, thanks for heads up. I will be properly revisiting everything. |
@kimsh02 For code formatting, you can check it locally using https://llvm.org/docs/Contributing.html#how-to-submit-a-patch Note: For the "Build and Test Linux" the current issue is not related to your PR |
@AmrDeveloper Thanks! |
Remove unused headers, apply camel case, CIR test null ptr. 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, Thanks
Let me know if you want help merging it 👍🏻 |
Fix #153658
Handle integral to pointer casts and port the relevant
cast.cpp
test cases from incubator.