Resolve test failures #17436
Resolve test failures #17436
Conversation
|
This is to resolve test failures after llvm/llvm-project@39ec9de |
|
@Fznamznon, could you please review this? I am not a 100% sure. |
| Src = Builder.CreateAddrSpaceCast( | ||
| Src, | ||
| llvm::PointerType::get(VMContext, DstTy->getPointerAddressSpace())); | ||
| SrcTy = Src->getType(); |
There was a problem hiding this comment.
Could you please clarify why this change is needed and why it was not needed before?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yeah, I was curious why didn't we hit the assertion before...
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We may leave this "as-is" to resolve failures
Oh absolutely, my comment is clearly out of scope of the PR ;)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Were you referring to the community FIXME below as "strange" or something else?
I was referring to the downstream code.
There was a problem hiding this comment.
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.
This is a cherry-pick of intel#17436 with slight modifications to `regcall-cc-test.cpp` to make it pass Patch-By: premanandrao <premanand.m.rao@intel.com>
No description provided.