[clang][SYCL] Fix libclc_call calling convention#21951
[clang][SYCL] Fix libclc_call calling convention#21951Fznamznon wants to merge 1 commit intointel:sycl-webfrom
Conversation
We have device-specific calling convention libclc_call here in the downstream added by 5b5fd84 which was not expected by the upstream patch 94ca490, so we ended up ignoring the calling convention attribute because the host target doesn't support it. This makes a little tweak in SemaDeclAttr to save the calling convention and also fixes sycl-cconv.cpp which was strangely half-merged after 94ca490.
|
I don't think I can push it upstream since there is no device-specific calling conventions out there. |
|
For reference, upstream commit 94ca490 is llvm/llvm-project@94ca490 ([clang][SYCL] Handle cdecl variadic functions for SYCL device) |
| else | ||
| A = TargetInfo::CCCR_OK; |
There was a problem hiding this comment.
I'm having difficulty concluding this is the right logic. If the result of TI.checkCallingConvention(CC) is not CCCR_OK and Aux is null (which should not be the case in practice except maybe for manual -cc1 invocations), should we be forcing A to CCCR_OK? I guess that is ok because we don't know the host target and so we just assume whatever it is supports the calling convention? That might be worth a comment. I guess I'm not sure why we are calling TI.checkCallingConvention() if we're just going to ignore the result anyway. Perhaps it will queue a deferred diagnostic for an unsupported calling convention?
There was a problem hiding this comment.
Perhaps it will queue a deferred diagnostic for an unsupported calling convention?
No, it won't.
So what happened here is that libclc_call calling convention was not supported for x86_64 host target, we checked it for aux and dropped it because it was not supported. So I retained the check for Aux but the calling convention should not be dropped if it is only accepted by device target. Eh, maybe we just should not check anything for device?
There was a problem hiding this comment.
So what happened here is that libclc_call calling convention was not supported for x86_64 host target, we checked it for aux and dropped it because it was not supported. So I retained the check for Aux but the calling convention should not be dropped if it is only accepted by device target.
Interesting, so this is the opposite case of what we usually have to deal with; a declaration that is valid for the device, but not for the host (in the form presented during device compilation). I presume the calling convention specifier was the result of a macro that is empty during host compilation, but expands to something interesting for device compilation.
Eh, maybe we just should not check anything for device?
That sounds right, but is also what the code was already doing, which I presume caused misbehavior that prompted this PR. Unfortunately, I don't think we really have enough information to make the right diagnosis at this point in the compilation since we don't know if the calling convention is intended to apply to host code, device code, or both. I guess the ideal solution would be to retain the calling convention and defer any diagnostics until we know whether the function will actually be emitted. Any chance the diagnostics issued below are of the eager variety and if we skip them now, that an appropriate diagnostic will be issued later if the function is actually used?
There was a problem hiding this comment.
That sounds right, but is also what the code was already doing
Not exactly, we did check for Aux target. I was suggesting not doing anything at all.
Any chance the diagnostics issued below are of the eager variety and if we skip them now, that an appropriate diagnostic will be issued later if the function is actually used?
This is problematic. Calling convention later becomes a part of function type and at the moment the function is called I don't think it can be changed. Now the problem is, the default behavior for some calling convention attributes is to drop the attribute and issue a warning, that behavior cannot be reproduced with deferred diagnostics due to calling convention being part of the function type, I think.
We have device-specific calling convention libclc_call here in the downstream added by 5b5fd84 which was not expected by the upstream patch 94ca490, so we ended up ignoring the calling convention attribute because the host target doesn't support it. This makes a little tweak in SemaDeclAttr to save the calling convention and also fixes sycl-cconv.cpp which was strangely half-merged after 94ca490.