Skip to content

[clang][SYCL] Fix libclc_call calling convention#21951

Open
Fznamznon wants to merge 1 commit intointel:sycl-webfrom
Fznamznon:fix-native-cpu-calling-conv
Open

[clang][SYCL] Fix libclc_call calling convention#21951
Fznamznon wants to merge 1 commit intointel:sycl-webfrom
Fznamznon:fix-native-cpu-calling-conv

Conversation

@Fznamznon
Copy link
Copy Markdown
Contributor

@Fznamznon Fznamznon commented May 7, 2026

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.

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.
@Fznamznon Fznamznon requested a review from a team as a code owner May 7, 2026 10:11
@Fznamznon
Copy link
Copy Markdown
Contributor Author

I don't think I can push it upstream since there is no device-specific calling conventions out there.
FYI @tahonermann

@tahonermann
Copy link
Copy Markdown
Contributor

For reference, upstream commit 94ca490 is llvm/llvm-project@94ca490 ([clang][SYCL] Handle cdecl variadic functions for SYCL device)

Comment on lines +5960 to +5961
else
A = TargetInfo::CCCR_OK;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants