-
Notifications
You must be signed in to change notification settings - Fork 182
[CIR] Backport TargetAddressSpaceAttr and Support both existing Lang AS and target AS attributes in pointer types and Global Ops
#1986
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
Draft
RiverDave
wants to merge
6
commits into
main
Choose a base branch
from
users/riverdave/backport-target-addrspace
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
+421
−273
Conversation
This file contains hidden or 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
…guage(clang) and target address-space attributes in pointer types
TargetAddressSpaceAttr and Support both existing AS and target AS attributes in pointer types and Global Ops
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions h,c,cpp,cl -- clang/include/clang/CIR/Dialect/Builder/CIRBaseBuilder.h clang/include/clang/CIR/Dialect/IR/CIRTypes.h clang/lib/CIR/CodeGen/CIRGenBuilder.h clang/lib/CIR/CodeGen/CIRGenBuiltin.cpp clang/lib/CIR/CodeGen/CIRGenCUDANV.cpp clang/lib/CIR/CodeGen/CIRGenDecl.cpp clang/lib/CIR/CodeGen/CIRGenExpr.cpp clang/lib/CIR/CodeGen/CIRGenExprScalar.cpp clang/lib/CIR/CodeGen/CIRGenModule.cpp clang/lib/CIR/CodeGen/CIRGenModule.h clang/lib/CIR/CodeGen/CIRGenTypeCache.h clang/lib/CIR/CodeGen/TargetInfo.cpp clang/lib/CIR/CodeGen/TargetInfo.h clang/lib/CIR/Dialect/IR/CIRAttrs.cpp clang/lib/CIR/Dialect/IR/CIRDialect.cpp clang/lib/CIR/Dialect/IR/CIRTypes.cpp clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp clang/test/CIR/CodeGen/HIP/address-spaces.cpp clang/test/CIR/CodeGen/OpenCL/addrspace-alloca.cl clang/test/CIR/CodeGen/OpenCL/array-decay.cl clang/test/CIR/CodeGen/OpenCL/global.cl clang/test/CIR/CodeGen/OpenCL/printf.cl clang/test/CIR/CodeGen/OpenCL/static-vardecl.cl clang/test/CIR/CodeGen/OpenCL/str_literals.cl clang/test/CIR/CodeGen/address-space-conversion.cpp clang/test/CIR/CodeGen/address-space.c --diff_from_common_commit
View the diff from clang-format here.diff --git a/clang/lib/CIR/CodeGen/CIRGenModule.cpp b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
index 5e022630f..4935322b5 100644
--- a/clang/lib/CIR/CodeGen/CIRGenModule.cpp
+++ b/clang/lib/CIR/CodeGen/CIRGenModule.cpp
@@ -1154,7 +1154,8 @@ CIRGenModule::getOrCreateCIRGlobal(StringRef mangledName, mlir::Type ty,
return entry;
}
- mlir::Attribute declCIRAS = cir::toCIRAddressSpaceAttr(&getMLIRContext(), getGlobalVarAddressSpace(d));
+ mlir::Attribute declCIRAS = cir::toCIRAddressSpaceAttr(
+ &getMLIRContext(), getGlobalVarAddressSpace(d));
// TODO(cir): do we need to strip pointer casts for Entry?
auto loc = getLoc(d->getSourceRange());
@@ -1977,7 +1978,8 @@ CIRGenModule::getAddrOfGlobalTemporary(const MaterializeTemporaryExpr *expr,
linkage = cir::GlobalLinkageKind::InternalLinkage;
}
}
- mlir::Attribute targetAS = cir::toCIRAddressSpaceAttr(&getMLIRContext(), addrSpace);
+ mlir::Attribute targetAS =
+ cir::toCIRAddressSpaceAttr(&getMLIRContext(), addrSpace);
auto loc = getLoc(expr->getSourceRange());
auto gv = createGlobalOp(*this, loc, name, type, isConstant, targetAS,
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 4256b89a2..eb8962d23 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -55,9 +55,9 @@ using namespace mlir;
#include "clang/CIR/Dialect/IR/CIROpsDialect.cpp.inc"
#include "clang/CIR/Interfaces/ASTAttrInterfaces.h"
#include "clang/CIR/Interfaces/CIROpInterfaces.h"
-#include <clang/CIR/MissingFeatures.h>
-#include <clang/CIR/Dialect/IR/CIRTypes.h>
#include <clang/CIR/Dialect/IR/CIRDialect.h>
+#include <clang/CIR/Dialect/IR/CIRTypes.h>
+#include <clang/CIR/MissingFeatures.h>
//===----------------------------------------------------------------------===//
// CIR Dialect
diff --git a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
index 080c6d9ba..7d3703afb 100644
--- a/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
+++ b/clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.cpp
@@ -379,8 +379,6 @@ void lowerAnnotationValue(
}
}
-
-
// Get addrspace by converting a pointer type.
// TODO: The approach here is a little hacky. We should access the target info
// directly to convert the address space of global op, similar to what we do
@@ -5030,9 +5028,9 @@ void prepareTypeConverter(mlir::LLVMTypeConverter &converter,
converter.addConversion([&,
lowerModule](cir::PointerType type) -> mlir::Type {
mlir::Attribute addrSpaceAttr = type.getAddrSpace();
- unsigned addrSpace = addrSpaceAttr
- ? getTargetAddrSpaceFromASAttr(addrSpaceAttr, lowerModule)
- : 0; // Default address space
+ unsigned addrSpace =
+ addrSpaceAttr ? getTargetAddrSpaceFromASAttr(addrSpaceAttr, lowerModule)
+ : 0; // Default address space
return mlir::LLVM::LLVMPointerType::get(type.getContext(), addrSpace);
});
converter.addConversion([&](cir::VPtrType type) -> mlir::Type {
|
TargetAddressSpaceAttr and Support both existing AS and target AS attributes in pointer types and Global OpsTargetAddressSpaceAttr and Support both existing Lang AS and target AS attributes in pointer types and Global Ops
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.
Bringing the changes we performed at: llvm/llvm-project#161028 down to the incubator isn't as straight-forward as I though. Therefore — this PR might be a bit complicated, bare with me :)
The main purpose of this is to bring the recent upstreamed
TargetAddressSpaceAttrand couple it withPointerTypeandGlobalOp. The main challenge is that this collides with the already implemented infrastructure related to AS that revolves around our unified enum approach. My main rationale here is that we let our new attribute to coexist with the already existingcir::AddressSpaceAttrso that we don't break any tests related to offload-type languages.Considering the above what I'm essentially doing is:
TargetAddressSpaceAttrhandle numeric target AS as it is done upstream.AddressSpaceAttr(which handles an enum) will handle language/clang specific AS (CUDA, OpenCL, etc..).PointerTypewill hold:TargetAddressSpaceAttr,AddressSpaceAttror null. We enforce this via a custom verifier. (I would've preferred to enforce this rule via an AttrConstraint, but apparently that doesn't support types? — At least I couldn't make it work.)GlobalOpswill enforce the rule mentioned above as well via AttrConstraints. This works since we can apply constraints to Ops.target_address_space(n)/clang_address_space(x).mlir::Attribute = {}However, not in the scope of this PR but my idea is to improve, revamp the later mentioned
AddressSpaceAttrand incorporate most of the feedback we received in: llvm/llvm-project#161028 (comment) — ideally to handle Language AS as I've mentioned. (And yes! I haven't forgotten about the {MemorySpaceAttrInterface} to be coupled with the ptr op).A couple of extra questions:
AddressSpaceAttr(to something likeClangAddressSpaceAttrorLangAddressSpaceAttr) since after this PR — Our previous attribute will no longer support target AS within that attribute?