-
Notifications
You must be signed in to change notification settings - Fork 138
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
[CIR][CIRGen] Enable address space flags and re-categorize / fix usages #614
Changes from all commits
b74b010
f84f187
6229950
d675216
7d911a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -173,7 +173,7 @@ static Address buildPointerWithAlignment(const Expr *E, | |||||
Addr = CGF.getBuilder().createElementBitCast( | ||||||
CGF.getLoc(E->getSourceRange()), Addr, ElemTy); | ||||||
if (CE->getCastKind() == CK_AddressSpaceConversion) { | ||||||
assert(!UnimplementedFeature::addressSpace()); | ||||||
assert(!UnimplementedFeature::addressSpaceCasting()); | ||||||
llvm_unreachable("NYI"); | ||||||
} | ||||||
return Addr; | ||||||
|
@@ -1206,23 +1206,21 @@ RValue CIRGenFunction::buildCall(clang::QualType CalleeType, | |||||
// some trivial cases]. | ||||||
// That is, in the general case, we should assume that a call through an | ||||||
// unprototyped function type works like a *non-variadic* call. The way we | ||||||
// make this work is to cast to the exxact type fo the promoted arguments. | ||||||
// make this work is to cast to the exact type fo the promoted arguments. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nittiest of picks - if you are already here:
Suggested change
|
||||||
// | ||||||
// Chain calls use the same code path to add the inviisble chain parameter to | ||||||
// Chain calls use the same code path to add the invisible chain parameter to | ||||||
// the function type. | ||||||
if (isa<FunctionNoProtoType>(FnType) || Chain) { | ||||||
assert(!UnimplementedFeature::chainCalls()); | ||||||
assert(!UnimplementedFeature::addressSpace()); | ||||||
auto CalleeTy = getTypes().GetFunctionType(FnInfo); | ||||||
// get non-variadic function type | ||||||
CalleeTy = mlir::cir::FuncType::get(CalleeTy.getInputs(), | ||||||
CalleeTy.getReturnType(), false); | ||||||
auto CalleePtrTy = | ||||||
mlir::cir::PointerType::get(builder.getContext(), CalleeTy); | ||||||
|
||||||
auto *Fn = Callee.getFunctionPointer(); | ||||||
mlir::Value Addr; | ||||||
if (auto funcOp = llvm::dyn_cast<mlir::cir::FuncOp>(Fn)) { | ||||||
assert(!UnimplementedFeature::addressSpaceInGlobalVar()); | ||||||
Addr = builder.create<mlir::cir::GetGlobalOp>( | ||||||
Comment on lines
+1223
to
1224
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it would make sense to add an assert here that the address space is the default (0)? Future users may try to understand why some verifier fail downstream from the codegen, and it would be hard to trace it back to this very low-level path in the code :) |
||||||
getLoc(E->getSourceRange()), | ||||||
mlir::cir::PointerType::get(builder.getContext(), | ||||||
|
@@ -1232,6 +1230,12 @@ RValue CIRGenFunction::buildCall(clang::QualType CalleeType, | |||||
Addr = Fn->getResult(0); | ||||||
} | ||||||
|
||||||
// Use the same addr space as Addr | ||||||
auto CalleePtrAS = | ||||||
Addr.getType().cast<mlir::cir::PointerType>().getAddrSpace(); | ||||||
auto CalleePtrTy = mlir::cir::PointerType::get(builder.getContext(), | ||||||
CalleeTy, CalleePtrAS); | ||||||
Comment on lines
+1236
to
+1237
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can you use |
||||||
|
||||||
Fn = builder.createBitcast(Addr, CalleePtrTy).getDefiningOp(); | ||||||
Callee.setFunctionPointer(Fn); | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -128,7 +128,6 @@ struct CIRGenTypeCache { | |
// Address spaces are not yet fully supported, but the usage of the default | ||
// alloca address space can be used for now only for comparison with the | ||
// default address space. | ||
assert(!UnimplementedFeature::addressSpace()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe you should switch this with |
||
assert(ASTAllocaAddressSpace == clang::LangAS::Default); | ||
return ASTAllocaAddressSpace; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir | ||
// RUN: FileCheck --input-file=%t.cir %s | ||
|
||
// CHECK: cir.func {{@.*foo.*}}(%arg0: !cir.ptr<!s32i> | ||
void foo(int __attribute__((address_space(0))) *arg) { | ||
return; | ||
} | ||
|
||
// CHECK: cir.func {{@.*bar.*}}(%arg0: !cir.ptr<!s32i, addrspace(1)> | ||
void bar(int __attribute__((address_space(1))) *arg) { | ||
return; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
// RUN: cir-translate %s -cir-to-llvmir -o %t.ll | ||
// RUN: FileCheck --input-file=%t.ll %s -check-prefix=LLVM | ||
|
||
!s32i = !cir.int<s, 32> | ||
|
||
module { | ||
// LLVM: define void @foo(ptr %0) | ||
cir.func @foo(%arg0: !cir.ptr<!s32i>) { | ||
// LLVM-NEXT: alloca ptr, | ||
%0 = cir.alloca !cir.ptr<!s32i>, !cir.ptr<!cir.ptr<!s32i>>, ["arg", init] {alignment = 8 : i64} | ||
cir.return | ||
} | ||
|
||
// LLVM: define void @bar(ptr addrspace(1) %0) | ||
cir.func @bar(%arg0: !cir.ptr<!s32i, addrspace(1)>) { | ||
// LLVM-NEXT: alloca ptr addrspace(1) | ||
%0 = cir.alloca !cir.ptr<!s32i, addrspace(1)>, !cir.ptr<!cir.ptr<!s32i, addrspace(1)>>, ["arg", init] {alignment = 8 : i64} | ||
cir.return | ||
} | ||
} |
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.
Clang's code sets the alloca's address space to the one described by the data-layout, and here you are using another path in the codegen - the
targetCIRGenInfo/targetCodeGenInfo
path. Not sure what's the right choice here, need to dive into a rabbit hole to understand the original intention of use for both.In general, we are trying to stick as much as possible to clang's codegen structure to avoid re-making past mistakes :D
I'll note that it do seem like I missed this details in the alloca builder I implemented a while ago! I guess we should either understand how to pass the datalayout here or add some nice TODO comment for future us :)
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.
For the two methods to get the alloca addrspace, I think these lines of comment helps. I noticed that the code is creating alloca and cast it immediately, therefore I naively simplify it to a direct alloca😂. Sorry for the mistake.
clangir/clang/lib/CodeGen/CGBuiltin.cpp
Lines 4082 to 4088 in 3da10fa
Most of targets in current codebase indeed implement
getASTAllocaAddressSpace
with DataLayout internally. The intention here should be to ensure the generality. (I don't fully understand the case of automatic variables mentioned in the comment above.)Anyway, shall we nicely have a
addressSpaceCasting()
mark here? For referenceDataLayout
directly, I know CIR uses the MLIR model, which has getAllocaMemorySpace method. It's not public throughCIRDataLayout
though, probably skipped at that time. This should be a workable path.😉