Skip to content

Resolve test failures #17436

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Mar 14, 2025
Merged

Resolve test failures #17436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion clang/lib/CodeGen/CGExprScalar.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2375,10 +2375,12 @@ Value *ScalarExprEmitter::VisitCastExpr(CastExpr *CE) {
llvm::Type *DstTy = ConvertType(DestTy);

if (SrcTy->isPointerTy() && DstTy->isPointerTy() &&
SrcTy->getPointerAddressSpace() != DstTy->getPointerAddressSpace())
SrcTy->getPointerAddressSpace() != DstTy->getPointerAddressSpace()) {
Src = Builder.CreateAddrSpaceCast(
Src,
llvm::PointerType::get(VMContext, DstTy->getPointerAddressSpace()));
SrcTy = Src->getType();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please clarify why this change is needed and why it was not needed before?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The AddressSpaceCast was added by us (@elizabethandrews in '22) but not upstreamed and the assert was added by community in '23. The reason we didn't need to make the change before I guess because we didn't trip the assertion.

The assertion feels right to me; the pointer types should have the same address space at that point. The thing is, we adjusted the Src expression above, but the SrcTy still refers to the unadjusted Src expression. Since the expression is reset, the type also should be, otherwise we are asserting something that has since changed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was curious why didn't we hit the assertion before...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The community change changes sret address space to that of the alloca address space, instead of the address space of the pointee. For the test case in CodeGenCXX/sret_cast_with_nonzero_alloca_as.cpp, this results in an AS of 0 instead of 4. (We cast it away in this case, but we didn't need to before.)

Copy link
Contributor

@Naghasan Naghasan Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, we adjusted the Src expression above, but the SrcTy still refers to the unadjusted Src expression.

Poping years after the patch, but this adjustment (for __builtin_alloca) doesn't look right here. It should probably be handled in the AST by adding the CK_AddressSpaceConversion or when we codegen the builtin by returning the cast address space. I think the 2nd option is more right as it is applying the SYCL address space deduction rules (the first makes assumption on the target address spaces in the AST).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may leave this "as-is" to resolve failures in pulldown and capture this

It should probably be handled in the AST by adding the CK_AddressSpaceConversion or when we codegen the builtin by returning the cast address space. I think the 2nd option is more right as it is applying the SYCL address space deduction rules (the first makes assumption on the target address spaces in the AST).

in a separate tracker.

The overall change of sret address space doesn't seem like a concern. It is this snippet of code is... strange.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may leave this "as-is" to resolve failures

Oh absolutely, my comment is clearly out of scope of the PR ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you referring to the community FIXME below as "strange" or something else?

I can file a tracker for a proper fix later on, so we can make progress here. If you and @Naghasan can add specifics to it, I would appreciate it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you referring to the community FIXME below as "strange" or something else?

I was referring to the downstream code.

Copy link
Contributor

@Naghasan Naghasan Mar 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the CreateAddrSpaceCast above (added in '22). I agree with the comment below, an address space cast shouldn't happen here and I think the one above is avoidable.

}

// FIXME: this is a gross but seemingly necessary workaround for an issue
// manifesting when a target uses a non-default AS for indirect sret args,
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGenSYCL/address-space-cond-op.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ struct S {
// CHECK-NEXT: br label [[COND_END]]
// CHECK: cond.end:
// CHECK-NEXT: [[COND_LVALUE:%.*]] = phi ptr addrspace(4) [ [[TMP1]], [[COND_TRUE]] ], [ [[RHS_ASCAST]], [[COND_FALSE]] ]
// CHECK-NEXT: call void @llvm.memcpy.p4.p4.i64(ptr addrspace(4) align 2 [[AGG_RESULT:%.*]], ptr addrspace(4) align 2 [[COND_LVALUE]], i64 2, i1 false)
// CHECK-NEXT: call void @llvm.memcpy.p0.p4.i64(ptr align 2 [[AGG_RESULT:%.*]], ptr addrspace(4) align 2 [[COND_LVALUE]], i64 2, i1 false)
// CHECK-NEXT: ret void
//
S foo(bool cond, S &lhs, S rhs) {
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGenSYCL/address-space-of-returns.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ A ret_agg() {
A a;
return a;
}
// CHECK: define {{.*}}spir_func void @{{.*}}ret_agg{{.*}}(ptr addrspace(4) dead_on_unwind noalias writable sret(%struct{{.*}}.A) align 4 %agg.result)
// CHECK: define {{.*}}spir_func void @{{.*}}ret_agg{{.*}}(ptr dead_on_unwind noalias writable sret(%struct{{.*}}.A) align 4 %agg.result)

template <typename name, typename Func>
__attribute__((sycl_kernel)) void kernel_single_task(const Func &kernelFunc) {
Expand Down
6 changes: 3 additions & 3 deletions clang/test/CodeGenSYCL/nvptx-short-ptr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@

// Targeting a 32-bit NVPTX, check that we see universal 32-bit pointers (the
// option changes nothing)
// CHECK32: target datalayout = "e-p:32:32-i64:64-i128:128-v16:16-v32:32-n16:32:64"
// CHECK32: target datalayout = "e-p:32:32-p6:32:32-i64:64-i128:128-v16:16-v32:32-n16:32:64"

// Targeting a 64-bit NVPTX target, check that we see 32-bit pointers for
// shared (3), const (4), and local (5) address spaces only.
// CHECK64-DEFAULT: target datalayout = "e-i64:64-i128:128-v16:16-v32:32-n16:32:64"
// CHECK64-SHORT: target datalayout = "e-p3:32:32-p4:32:32-p5:32:32-i64:64-i128:128-v16:16-v32:32-n16:32:64"
// CHECK64-DEFAULT: target datalayout = "e-p6:32:32-i64:64-i128:128-v16:16-v32:32-n16:32:64"
// CHECK64-SHORT: target datalayout = "e-p3:32:32-p4:32:32-p5:32:32-p6:32:32-i64:64-i128:128-v16:16-v32:32-n16:32:64"
2 changes: 1 addition & 1 deletion clang/test/CodeGenSYCL/regcall-cc-test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ struct NonCopyable {
// CHECK-DAG: %struct.NonCopyable = type { i32 }

SYCL_DEVICE int __regcall bar(NonCopyable x) {
// CHECK-DAG: define dso_local x86_regcallcc noundef i32 @_Z15__regcall3__bar11NonCopyable(ptr noundef %x)
// CHECK-DAG: define dso_local x86_regcallcc noundef i32 @_Z15__regcall3__bar11NonCopyable(ptr noundef byval(%struct.NonCopyable) align 4 %x)
return x.a;
}

Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGenSYCL/sycl-intelfpga-bitint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

#include "Inputs/sycl.hpp"

// CHECK: define{{.*}} void @_Z3fooDB4096_S_(ptr addrspace(4) {{.*}} sret(i4096) align 8 %agg.result, ptr {{.*}} byval(i4096) align 8 %[[ARG1:[0-9]+]], ptr {{.*}} byval(i4096) align 8 %[[ARG2:[0-9]+]])
// CHECK: define{{.*}} void @_Z3fooDB4096_S_(ptr {{.*}} sret(i4096) align 8 %agg.result, ptr {{.*}} byval(i4096) align 8 %[[ARG1:[0-9]+]], ptr {{.*}} byval(i4096) align 8 %[[ARG2:[0-9]+]])
signed _BitInt(4096) foo(signed _BitInt(4096) a, signed _BitInt(4096) b) {
// CHECK: %a.addr.ascast = addrspacecast ptr %a.addr to ptr addrspace(4)
// CHECK: %b.addr.ascast = addrspacecast ptr %b.addr to ptr addrspace(4)
Expand Down