[Clang][CodeGen] Start migrating away from assuming the Default AS is 0 #88182
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -368,7 +368,8 @@ CodeGenModule::CodeGenModule(ASTContext &C, | |
| IntTy = llvm::IntegerType::get(LLVMContext, C.getTargetInfo().getIntWidth()); | ||
| IntPtrTy = llvm::IntegerType::get(LLVMContext, | ||
| C.getTargetInfo().getMaxPointerWidth()); | ||
| Int8PtrTy = llvm::PointerType::get(LLVMContext, | ||
arichardson marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. This seems to cause regressions for HIP -fsanitize=address. Basically rocm device library for ASAN is compiled from OpenCL source code, for which the default address space is mapped to addr space 5 in IR. Int8PtrTy is used to determine the pointer type in the llvm.compiler.used global array, which causes the pointers in that array to be in addr space 5. However, for HIP those are in addr space 0. The variable from different bitcode are appended together by the linker. The linker checks their type and emits error since they do not match. https://godbolt.org/z/s48fTj7Pv Before this change, the pointers are in addr space 0 for both HIP and OpenCL, therefore no such issue. There was a problem hiding this comment. This seems to be an error on ASAN's side, it's relying on non-guaranteed behaviour and linking BC emanating from different languages with different AS maps. To wit, 0 is private for OCL, so having There was a problem hiding this comment. The AMDGPU address space mapping is at https://llvm.org/docs/AMDGPUUsage.html#address-spaces and address space 0 is not private. The address space for all languages compiled for the AMDGPU target is consistent. There was a problem hiding this comment. The IR address space is a pure target concept. Any address space error would be on the frontend emitting the wrong IR for the target There was a problem hiding this comment. Fixing emitUsed probably is a feasible solution, but GlobalsInt8PtrTy will cause the used array containing pointers to addr space 1, which is not backward compatible for HIP. Maybe just use pointer to addr space 0 type as before. There was a problem hiding this comment. I believe there is special case linker code to handle mismatched address spaces for the intrinsic globals, so I don't think changing this would break anything. The address space of llvm.used doesn't really mean anything There was a problem hiding this comment.
This would be wrong, assuming that 0 / Unqual is generally equivalent with some form of generic pointer is the core problem here. I'm not exactly sure what you mean by "backward compatible" here though, both the HIP source and whatever BC gets linked should go through the same toolchain, surely? Also, what @arsenm said sounds about right. There was a problem hiding this comment.
I don't think mixing languages with different LangAS maps is sound, OCL originating BC is not generic BC. There's no "error" here, the FE is doing what is asked of it (indiscriminately assuming that AS 0 is some generic/flat pointer that is valid everywhere is rather risque, as per prior conversation in this review). The actual problem is, AFAICT, that when we call There was a problem hiding this comment.
It is entirely sound, and required for this entire system to work (e.g. we implement the libraries in OpenCL used by all the languages). The point of the LangAS is to map to the target address space, which does not care what the language is. Different languages should be trying to be ABI compatible with each other, which includes emitting the correct IR address space There was a problem hiding this comment.
I am aware of what we decided to do. Whether or not that is actually sound is a different kettle of fish, but let's agree to disagree and move on. Stepping back, I'm trying to figure out what the actual suggestion is. Reverting the change is wrong, because there are other targets where for which 0 is definitely not a sound default, and this impacts more than the
If there's some third reasonable solution, I apologise for missing it - please share! |
||
| C.getTargetAddressSpace(LangAS::Default)); | ||
| const llvm::DataLayout &DL = M.getDataLayout(); | ||
| AllocaInt8PtrTy = | ||
| llvm::PointerType::get(LLVMContext, DL.getAllocaAddrSpace()); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,127 @@ | ||
| // NOTE: Assertions have been autogenerated by utils/update_cc_test_checks.py UTC_ARGS: --check-globals all --no-generate-body-for-unused-prefixes --version 4 | ||
| // RUN: %clang_cc1 -I%S %s -triple amdgcn-amd-amdhsa -emit-llvm -fcxx-exceptions -fexceptions -o - | FileCheck %s | ||
| // RUN: %clang_cc1 -I%S %s -triple spirv64-unknown-unknown -fsycl-is-device -emit-llvm -fcxx-exceptions -fexceptions -o - | FileCheck %s --check-prefix=WITH-NONZERO-DEFAULT-AS | ||
|
|
||
| struct A { virtual void f(); }; | ||
| struct B : A { }; | ||
|
|
||
| B fail; | ||
| //. | ||
| // CHECK: @_ZTV1B = linkonce_odr unnamed_addr addrspace(1) constant { [3 x ptr addrspace(1)] } { [3 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) @_ZTI1B, ptr addrspace(1) addrspacecast (ptr @_ZN1A1fEv to ptr addrspace(1))] }, comdat, align 8 | ||
| // CHECK: @fail = addrspace(1) global { ptr addrspace(1) } { ptr addrspace(1) getelementptr inbounds inrange(-16, 8) ({ [3 x ptr addrspace(1)] }, ptr addrspace(1) @_ZTV1B, i32 0, i32 0, i32 2) }, align 8 | ||
| // CHECK: @_ZTI1A = external addrspace(1) constant ptr addrspace(1) | ||
| // CHECK: @_ZTVN10__cxxabiv120__si_class_type_infoE = external addrspace(1) global [0 x ptr addrspace(1)] | ||
| // CHECK: @_ZTS1B = linkonce_odr addrspace(1) constant [3 x i8] c"1B\00", comdat, align 1 | ||
| // CHECK: @_ZTI1B = linkonce_odr addrspace(1) constant { ptr addrspace(1), ptr addrspace(1), ptr addrspace(1) } { ptr addrspace(1) getelementptr inbounds (ptr addrspace(1), ptr addrspace(1) @_ZTVN10__cxxabiv120__si_class_type_infoE, i64 2), ptr addrspace(1) @_ZTS1B, ptr addrspace(1) @_ZTI1A }, comdat, align 8 | ||
| // CHECK: @__oclc_ABI_version = weak_odr hidden local_unnamed_addr addrspace(4) constant i32 500 | ||
| //. | ||
| // WITH-NONZERO-DEFAULT-AS: @_ZTV1B = linkonce_odr unnamed_addr addrspace(1) constant { [3 x ptr addrspace(1)] } { [3 x ptr addrspace(1)] [ptr addrspace(1) null, ptr addrspace(1) @_ZTI1B, ptr addrspace(1) addrspacecast (ptr @_ZN1A1fEv to ptr addrspace(1))] }, comdat, align 8 | ||
| // WITH-NONZERO-DEFAULT-AS: @fail = addrspace(1) global { ptr addrspace(1) } { ptr addrspace(1) getelementptr inbounds inrange(-16, 8) ({ [3 x ptr addrspace(1)] }, ptr addrspace(1) @_ZTV1B, i32 0, i32 0, i32 2) }, align 8 | ||
| // WITH-NONZERO-DEFAULT-AS: @_ZTI1A = external addrspace(1) constant ptr addrspace(1) | ||
| // WITH-NONZERO-DEFAULT-AS: @_ZTVN10__cxxabiv120__si_class_type_infoE = external addrspace(1) global [0 x ptr addrspace(1)] | ||
| // WITH-NONZERO-DEFAULT-AS: @_ZTS1B = linkonce_odr addrspace(1) constant [3 x i8] c"1B\00", comdat, align 1 | ||
| // WITH-NONZERO-DEFAULT-AS: @_ZTI1B = linkonce_odr addrspace(1) constant { ptr addrspace(1), ptr addrspace(1), ptr addrspace(1) } { ptr addrspace(1) getelementptr inbounds (ptr addrspace(1), ptr addrspace(1) @_ZTVN10__cxxabiv120__si_class_type_infoE, i64 2), ptr addrspace(1) @_ZTS1B, ptr addrspace(1) @_ZTI1A }, comdat, align 8 | ||
| //. | ||
| // CHECK-LABEL: define dso_local noundef nonnull align 8 dereferenceable(8) ptr @_Z1fP1A( | ||
| // CHECK-SAME: ptr noundef [[A:%.*]]) #[[ATTR0:[0-9]+]] personality ptr @__gxx_personality_v0 { | ||
| // CHECK-NEXT: entry: | ||
| // CHECK-NEXT: [[RETVAL:%.*]] = alloca ptr, align 8, addrspace(5) | ||
| // CHECK-NEXT: [[A_ADDR:%.*]] = alloca ptr, align 8, addrspace(5) | ||
| // CHECK-NEXT: [[EXN_SLOT:%.*]] = alloca ptr, align 8, addrspace(5) | ||
| // CHECK-NEXT: [[EHSELECTOR_SLOT:%.*]] = alloca i32, align 4, addrspace(5) | ||
| // CHECK-NEXT: [[RETVAL_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[RETVAL]] to ptr | ||
| // CHECK-NEXT: [[A_ADDR_ASCAST:%.*]] = addrspacecast ptr addrspace(5) [[A_ADDR]] to ptr | ||
| // CHECK-NEXT: store ptr [[A]], ptr [[A_ADDR_ASCAST]], align 8 | ||
| // CHECK-NEXT: [[TMP0:%.*]] = load ptr, ptr [[A_ADDR_ASCAST]], align 8 | ||
| // CHECK-NEXT: [[TMP1:%.*]] = call ptr @__dynamic_cast(ptr [[TMP0]], ptr addrspace(1) @_ZTI1A, ptr addrspace(1) @_ZTI1B, i64 0) #[[ATTR3:[0-9]+]] | ||
| // CHECK-NEXT: [[TMP2:%.*]] = icmp eq ptr [[TMP1]], null | ||
| // CHECK-NEXT: br i1 [[TMP2]], label [[DYNAMIC_CAST_BAD_CAST:%.*]], label [[DYNAMIC_CAST_END:%.*]] | ||
| // CHECK: dynamic_cast.bad_cast: | ||
| // CHECK-NEXT: invoke void @__cxa_bad_cast() #[[ATTR4:[0-9]+]] | ||
| // CHECK-NEXT: to label [[INVOKE_CONT:%.*]] unwind label [[LPAD:%.*]] | ||
| // CHECK: invoke.cont: | ||
| // CHECK-NEXT: unreachable | ||
| // CHECK: dynamic_cast.end: | ||
| // CHECK-NEXT: br label [[TRY_CONT:%.*]] | ||
| // CHECK: lpad: | ||
| // CHECK-NEXT: [[TMP3:%.*]] = landingpad { ptr, i32 } | ||
| // CHECK-NEXT: catch ptr null | ||
| // CHECK-NEXT: [[TMP4:%.*]] = extractvalue { ptr, i32 } [[TMP3]], 0 | ||
| // CHECK-NEXT: store ptr [[TMP4]], ptr addrspace(5) [[EXN_SLOT]], align 8 | ||
| // CHECK-NEXT: [[TMP5:%.*]] = extractvalue { ptr, i32 } [[TMP3]], 1 | ||
| // CHECK-NEXT: store i32 [[TMP5]], ptr addrspace(5) [[EHSELECTOR_SLOT]], align 4 | ||
| // CHECK-NEXT: br label [[CATCH:%.*]] | ||
| // CHECK: catch: | ||
| // CHECK-NEXT: [[EXN:%.*]] = load ptr, ptr addrspace(5) [[EXN_SLOT]], align 8 | ||
| // CHECK-NEXT: [[TMP6:%.*]] = call ptr @__cxa_begin_catch(ptr [[EXN]]) #[[ATTR3]] | ||
| // CHECK-NEXT: call void @__cxa_end_catch() | ||
| // CHECK-NEXT: br label [[TRY_CONT]] | ||
| // CHECK: try.cont: | ||
| // CHECK-NEXT: ret ptr addrspacecast (ptr addrspace(1) @fail to ptr) | ||
| // | ||
| // WITH-NONZERO-DEFAULT-AS-LABEL: define spir_func noundef align 8 dereferenceable(8) ptr addrspace(4) @_Z1fP1A( | ||
| // WITH-NONZERO-DEFAULT-AS-SAME: ptr addrspace(4) noundef [[A:%.*]]) #[[ATTR0:[0-9]+]] personality ptr @__gxx_personality_v0 { | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: entry: | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[RETVAL:%.*]] = alloca ptr addrspace(4), align 8 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[A_ADDR:%.*]] = alloca ptr addrspace(4), align 8 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[EXN_SLOT:%.*]] = alloca ptr addrspace(4), align 8 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[EHSELECTOR_SLOT:%.*]] = alloca i32, align 4 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[RETVAL_ASCAST:%.*]] = addrspacecast ptr [[RETVAL]] to ptr addrspace(4) | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[A_ADDR_ASCAST:%.*]] = addrspacecast ptr [[A_ADDR]] to ptr addrspace(4) | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: store ptr addrspace(4) [[A]], ptr addrspace(4) [[A_ADDR_ASCAST]], align 8 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[TMP0:%.*]] = load ptr addrspace(4), ptr addrspace(4) [[A_ADDR_ASCAST]], align 8 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[TMP1:%.*]] = call spir_func ptr addrspace(4) @__dynamic_cast(ptr addrspace(4) [[TMP0]], ptr addrspace(1) @_ZTI1A, ptr addrspace(1) @_ZTI1B, i64 0) #[[ATTR3:[0-9]+]] | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[TMP2:%.*]] = icmp eq ptr addrspace(4) [[TMP1]], null | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: br i1 [[TMP2]], label [[DYNAMIC_CAST_BAD_CAST:%.*]], label [[DYNAMIC_CAST_END:%.*]] | ||
| // WITH-NONZERO-DEFAULT-AS: dynamic_cast.bad_cast: | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: invoke spir_func void @__cxa_bad_cast() #[[ATTR4:[0-9]+]] | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: to label [[INVOKE_CONT:%.*]] unwind label [[LPAD:%.*]] | ||
| // WITH-NONZERO-DEFAULT-AS: invoke.cont: | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: unreachable | ||
| // WITH-NONZERO-DEFAULT-AS: dynamic_cast.end: | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: br label [[TRY_CONT:%.*]] | ||
| // WITH-NONZERO-DEFAULT-AS: lpad: | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[TMP3:%.*]] = landingpad { ptr addrspace(4), i32 } | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: catch ptr addrspace(4) null | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[TMP4:%.*]] = extractvalue { ptr addrspace(4), i32 } [[TMP3]], 0 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: store ptr addrspace(4) [[TMP4]], ptr [[EXN_SLOT]], align 8 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[TMP5:%.*]] = extractvalue { ptr addrspace(4), i32 } [[TMP3]], 1 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: store i32 [[TMP5]], ptr [[EHSELECTOR_SLOT]], align 4 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: br label [[CATCH:%.*]] | ||
| // WITH-NONZERO-DEFAULT-AS: catch: | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[EXN:%.*]] = load ptr addrspace(4), ptr [[EXN_SLOT]], align 8 | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: [[TMP6:%.*]] = call spir_func ptr addrspace(4) @__cxa_begin_catch(ptr addrspace(4) [[EXN]]) #[[ATTR3]] | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: call spir_func void @__cxa_end_catch() | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: br label [[TRY_CONT]] | ||
| // WITH-NONZERO-DEFAULT-AS: try.cont: | ||
| // WITH-NONZERO-DEFAULT-AS-NEXT: ret ptr addrspace(4) addrspacecast (ptr addrspace(1) @fail to ptr addrspace(4)) | ||
| // | ||
| const B& f(A *a) { | ||
| try { | ||
| dynamic_cast<const B&>(*a); | ||
| } catch (...) { | ||
| } | ||
| return fail; | ||
| } | ||
|
|
||
|
|
||
| //. | ||
| // CHECK: attributes #[[ATTR0]] = { mustprogress noinline optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" } | ||
| // CHECK: attributes #[[ATTR1:[0-9]+]] = { nounwind willreturn memory(read) } | ||
| // CHECK: attributes #[[ATTR2:[0-9]+]] = { "no-trapping-math"="true" "stack-protector-buffer-size"="8" } | ||
| // CHECK: attributes #[[ATTR3]] = { nounwind } | ||
| // CHECK: attributes #[[ATTR4]] = { noreturn } | ||
| //. | ||
| // WITH-NONZERO-DEFAULT-AS: attributes #[[ATTR0]] = { convergent mustprogress noinline norecurse nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" } | ||
| // WITH-NONZERO-DEFAULT-AS: attributes #[[ATTR1:[0-9]+]] = { nounwind willreturn memory(read) } | ||
| // WITH-NONZERO-DEFAULT-AS: attributes #[[ATTR2:[0-9]+]] = { convergent nounwind "no-trapping-math"="true" "stack-protector-buffer-size"="8" } | ||
| // WITH-NONZERO-DEFAULT-AS: attributes #[[ATTR3]] = { nounwind } | ||
| // WITH-NONZERO-DEFAULT-AS: attributes #[[ATTR4]] = { noreturn } | ||
| //. | ||
| // CHECK: [[META0:![0-9]+]] = !{i32 1, !"amdhsa_code_object_version", i32 500} | ||
| // CHECK: [[META1:![0-9]+]] = !{i32 1, !"wchar_size", i32 4} | ||
| // CHECK: [[META2:![0-9]+]] = !{!"{{.*}}clang version {{.*}}"} | ||
| //. | ||
| // WITH-NONZERO-DEFAULT-AS: [[META0:![0-9]+]] = !{i32 1, !"wchar_size", i32 4} | ||
| // WITH-NONZERO-DEFAULT-AS: [[META1:![0-9]+]] = !{!"{{.*}}clang version {{.*}}"} | ||
| //. |
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.
Should this be
GlobalsInt8PtrTy?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.
It should but sadly it cannot, see our historical conversation here: https://reviews.llvm.org/D157452. I've not got around to working on your suggestion there about supporting declaring a default AS for a class, so we have to keep things like so for now.
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.
I also find this somewhat surprising. Looking at the discussion that linked review the concern seems to be that you'd end up emitting e.g.
ptr addrspace(1)here and users of EmitCXXTypeidLValue do not expect to handle that?Could you add a comment that the result of this type is used in contexts where the "default" address space is expected. Without the comment I find this very confusing.
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.
Done-ish, please let me know if this is not actually helpful in clarifying and I can refine / reword.
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.
TODO seems like a strange way to phrase this if it's true that this can never be GlobalsInt8PtrTy
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.
It could be
GlobalsInt8PtrTy, but the main roadblock would require something that @rjmccall suggested looking into, namely adding the ability to declare a default AS for a class type, which stdlib implementations could then re-use. Having said that, I've neither had the time to look into it, nor figured out if this would work in general, considering we need to compose with stdlib implementations other than libc++. Having said that, perhaps the TODO: should actually be at the end of the comment, and just say something along the lines of "investigate if it is possible to remove this limitation"?