This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Disable noundef attribute for languages which allow uninitialized function arguments
AbandonedPublic

Authored by skc7 on Jun 30 2022, 5:50 AM.

Details

Summary

Languages like CUDA, HIP etc. have APIs which accept uninitialized function arguments.
With D105169, noundef-analysis has been enabled by default and
we are forced to assume very strict constraints for the mentioned languages.
So, the proposed change is to skip adding noundef attribute to function
arguments and return values for such languages.

Diff Detail

Event Timeline

skc7 created this revision.Jun 30 2022, 5:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 5:50 AM
skc7 requested review of this revision.Jun 30 2022, 5:50 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
arsenm added inline comments.Jun 30 2022, 7:11 AM
clang/include/clang/Basic/LangOptions.h
534

Should add a comment explaining it here

skc7 updated this revision to Diff 441409.Jun 30 2022, 8:26 AM

Add description for allowUninitializedFunctionsArgs

The current set of reviewers is mostly loaded with HIP engineers who are familiar with the issue and the proposed solution. But this solution affects all languages with convergent functions, which is visible from the affected tests. It will be good to seek comments from people responsible for CUDA, OpenMP and OpenCL too.

clang/include/clang/Basic/LangOptions.h
534

The spelling should be "allowUninitializedFunctionArgs()", where the noun "Function" is singular.

clang/lib/CodeGen/CGCall.cpp
2306

This comment is merely an English translation of the Boolean expression. It should instead provide a bit of context. Like the fact that on HIP, CUDA, etc, some functions have multi-threaded semantics where it is enough for only one or some threads to provide defined arguments. Depending on semantics, undef arguments in some threads don't produce undefined results in the function call.

It might be worth adding this longer explanation to the commit description too, rather than just saying "strict constraints".

jdoerfert requested changes to this revision.Jul 13 2022, 10:07 PM

Wasn't there a discussion about this before, or some other patch? If so, could we please link such things (e.g., in the commit message) so people don't have to remember and find the links themselves.

FWIW, I don't think this is the right approach. For one, assuming we want such a logic, I would expect the default of the flag to change and not some hidden stuff that makes the still exposed flag meaningless.
That said, I doubt this is even what we want. Throwing away the benefits of the noundef for one special case. IIRC, I mentioned alternatives in the other discussion already,... not that I have a link handy.

This revision now requires changes to proceed.Jul 13 2022, 10:07 PM
skc7 updated this revision to Diff 444657.Jul 14 2022, 7:45 AM

Rebase and fix for review comments.

That said, I doubt this is even what we want. Throwing away the benefits of the noundef for one special case. IIRC, I mentioned alternatives in the other discussion already,... not that I have a link handy.

I've spent a while thinking about this and don't see a better option. It's one special case, but since that special case can be called from arbitrary user code, I don't see how we can preserve noundef. We can't specially treat the handful of special intrinsics/operands for this, since we would transitively need to apply this to any user code which calls those functions

That said, I doubt this is even what we want. Throwing away the benefits of the noundef for one special case. IIRC, I mentioned alternatives in the other discussion already,... not that I have a link handy.

I've spent a while thinking about this and don't see a better option. It's one special case, but since that special case can be called from arbitrary user code, I don't see how we can preserve noundef. We can't specially treat the handful of special intrinsics/operands for this, since we would transitively need to apply this to any user code which calls those functions

I guess, that is the actual question here: Does CUDA/HIP overwrite the semantics of the base language entirely or make exceptions for the intrinsics they added.
From what I can tell, CUDA/HIP did the latter. I am not sure how much their standards (if any) mention this.

OpenMP offloading, for example, does not overwrite the base language (C/C++). For OpenMP we would want to make the intrinsics special but that's it. We would even be fine without any special handling as the only uses are in our runtime (or other clang shipped code).

If the HIP/CUDA people think we should give up on noundef arguments completely, let's do it by language rather than some implicit property (convergent). OpenCL & SYCL people should also comment. (ping @tra, @RaviNarayanaswamy, @bader, @Anastasia)

That said, I doubt this is even what we want. Throwing away the benefits of the noundef for one special case. IIRC, I mentioned alternatives in the other discussion already,... not that I have a link handy.

I've spent a while thinking about this and don't see a better option. It's one special case, but since that special case can be called from arbitrary user code, I don't see how we can preserve noundef. We can't specially treat the handful of special intrinsics/operands for this, since we would transitively need to apply this to any user code which calls those functions

I guess, that is the actual question here: Does CUDA/HIP overwrite the semantics of the base language entirely or make exceptions for the intrinsics they added.
From what I can tell, CUDA/HIP did the latter. I am not sure how much their standards (if any) mention this.

As far as I can tell there's no explicit mention of this. However, there is apparently code in the wild where people are relying on this behavior. It seems almost reasonable, but I do wish this were defined.

OpenMP offloading, for example, does not overwrite the base language (C/C++). For OpenMP we would want to make the intrinsics special but that's it. We would even be fine without any special handling as the only uses are in our runtime (or other clang shipped code).

If the HIP/CUDA people think we should give up on noundef arguments completely, let's do it by language rather than some implicit property (convergent). OpenCL & SYCL people should also comment. (ping @tra, @RaviNarayanaswamy, @bader, @Anastasia)

I do think we could go backwards and infer noundef like any other attribute, and take care to avoid inferring it in the presence of these intrinsics. I thought it was weird clang was adding these upfront in the first place.

The consensus in the meeting was that we should try introducing a new argument attribute for values that are allowed to use undef values. It would need to be applied to the builtins and any wrapper functions for them. This would leave the normal language undefined behavior for arbitrary arguments

Can this be abandoned now?

skc7 abandoned this revision.Sep 29 2022, 8:10 PM