This is an archive of the discontinued LLVM Phabricator instance.

Differential D128907

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

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

Details

Reviewers
arsenm
sameerds
cdevadas
ronlieb
yaxunl
b-sumner
bcahoon
jdoerfert
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

Unit TestsFailed

TimeTest
100 msx64 debian > LLVM.tools/llvm-ar::bitcode.ll

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
Herald added subscribers: kosarev, mattd, asavonic and 6 others. · View Herald Transcript
skc7 requested review of this revision.Jun 30 2022, 5:50 AM
Herald added a reviewer: jdoerfert. · View Herald TranscriptJun 30 2022, 5:50 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, cfe-commits, sstefan1, wdng. · View Herald Transcript
skc7 mentioned this in D128700: [AMDGPU][Clang] Skip adding noundef attribute to AMDGPU HIP device functions.Jun 30 2022, 5:56 AM
arsenm added inline comments.Jun 30 2022, 7:11 AM
clang/include/clang/Basic/LangOptions.h
534

Should add a comment explaining it here

Harbormaster completed remote builds in B173019: Diff 441370.Jun 30 2022, 7:15 AM
skc7 updated this revision to Diff 441409.Jun 30 2022, 8:26 AM

Add description for allowUninitializedFunctionsArgs

Harbormaster completed remote builds in B173050: Diff 441409.Jun 30 2022, 8:26 AM
msearles added a subscriber: msearles.Jul 13 2022, 9:33 PM
sameerds added a comment.Jul 13 2022, 9:44 PM

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
2309

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.

arsenm added a comment.Jul 14 2022, 7:47 AM
In D128907#3650750, @jdoerfert wrote:

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

jdoerfert added subscribers: RaviNarayanaswamy, Anastasia, tra, bader.Jul 14 2022, 8:08 AM
In D128907#3652077, @arsenm wrote:
In D128907#3650750, @jdoerfert wrote:

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)

arsenm added a comment.Jul 14 2022, 8:16 AM
In D128907#3652140, @jdoerfert wrote:
In D128907#3652077, @arsenm wrote:
In D128907#3650750, @jdoerfert wrote:

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.

jdoerfert added a comment.Jul 14 2022, 8:27 AM

@arsenm put this on the GPU working group agenda for tomorrow: https://docs.google.com/document/d/1m_oSe1HwtWdQ2JUmMRTAVHbUS7Dv4MRsqptiYcgK6iI/edit?usp=sharing
Join in if you can!

Harbormaster completed remote builds in B175404: Diff 444657.Jul 14 2022, 11:11 AM
arsenm added a comment.Jul 16 2022, 7:50 AM

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

skc7 mentioned this in D130224: [Clang][Attribute] Introduce maybe_undef attribute for function arguments which accepts undef values.Jul 21 2022, 12:17 AM
arsenm added a comment.Sep 28 2022, 12:43 PM

Can this be abandoned now?

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

Large Diff

This large diff affects 108 files. Files without inline comments have been collapsed. Expand All Files

Revision Contents

PathSize
clang/
include/
clang/
Basic/
7 lines
lib/
CodeGen/
22 lines
test/
CodeGen/
PowerPC/
2 lines
CodeGenCUDA/
2 lines
2 lines
2 lines
2 lines
8 lines
4 lines
2 lines
4 lines
8 lines
35 lines
2 lines
CodeGenCUDASPIRV/
3 lines
CodeGenHIP/
10 lines
38 lines
CodeGenOpenCL/
20 lines
10 lines
8 lines
48 lines
2 lines
6 lines
26 lines
54 lines
12 lines
4 lines
2 lines
2 lines
6 lines
4 lines
8 lines
12 lines
8 lines
4 lines
20 lines
60 lines
10 lines
CodeGenOpenCLCXX/
2 lines
4 lines
2 lines
32 lines
4 lines
2 lines
22 lines
6 lines
CodeGenSYCL/
52 lines
16 lines
2 lines
40 lines
OpenMP/
2 lines
16 lines
6 lines
4 lines
12 lines
2 lines
4 lines
1106 lines
5166 lines
8 lines
8 lines
26 lines
4 lines
355 lines
420 lines
90 lines
179 lines
52 lines
6 lines
8 lines
444 lines
457 lines
18 lines
4 lines
197 lines
134 lines
1642 lines
1216 lines
1086 lines
32 lines
162 lines
4 lines
2 lines
8 lines
12 lines
6491 lines
24 lines
6382 lines
24 lines
7566 lines
3591 lines
2945 lines
4 lines
2 lines
8625 lines
11799 lines
926 lines
1683 lines
1150 lines
1669 lines
1739 lines
1206 lines
7962 lines
1294 lines
2945 lines
3050 lines
254 lines
llvm/
test/
Transforms/
SimplifyCFG/
31 lines

Diff 444657

clang/include/clang/Basic/LangOptions.h

Show First 20 Lines • Show All 524 Lines • ▼ Show 20 Lines#include "clang/Basic/LangOptions.def"
bool allowsNonTrivialObjCLifetimeQualifiers() const { bool allowsNonTrivialObjCLifetimeQualifiers() const {
return ObjCAutoRefCount || ObjCWeak; return ObjCAutoRefCount || ObjCWeak;
} }
bool assumeFunctionsAreConvergent() const { bool assumeFunctionsAreConvergent() const {
return ConvergentFunctions; return ConvergentFunctions;
} }
/// Return true if uninitialized function arguments are allowed.
bool allowUninitializedFunctionArgs() const {
arsenmUnsubmitted
Not Done
ReplyInline Actions

Should add a comment explaining it here

arsenm: Should add a comment explaining it here
sameerdsUnsubmitted
Not Done
ReplyInline Actions

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

sameerds: The spelling should be "allowUninitializedFunctionArgs()", where the noun "Function" is…
/// CUDA/HIP etc. cross-lane APIs are convergent functions
/// and accept uninitialised variables as arguments.
return ConvergentFunctions;
}
/// Return the OpenCL C or C++ version as a VersionTuple. /// Return the OpenCL C or C++ version as a VersionTuple.
VersionTuple getOpenCLVersionTuple() const; VersionTuple getOpenCLVersionTuple() const;
/// Return the OpenCL version that kernel language is compatible with /// Return the OpenCL version that kernel language is compatible with
unsigned getOpenCLCompatibleVersion() const; unsigned getOpenCLCompatibleVersion() const;
/// Return the OpenCL C or C++ for OpenCL language name and version /// Return the OpenCL C or C++ for OpenCL language name and version
/// as a string. /// as a string.
▲ Show 20 Lines • Show All 366 Lines • Show Last 20 Lines

clang/lib/CodeGen/CGCall.cpp

Show First 20 Lines • Show All 2,300 Lines • ▼ Show 20 Linesvoid CodeGenModule::ConstructAttributeList(StringRef Name,
// We don't want to be too aggressive with the return checking, unless // We don't want to be too aggressive with the return checking, unless
// it's explicit in the code opts or we're using an appropriate sanitizer. // it's explicit in the code opts or we're using an appropriate sanitizer.
// Try to respect what the programmer intended. // Try to respect what the programmer intended.
HasStrictReturn &= getCodeGenOpts().StrictReturn || HasStrictReturn &= getCodeGenOpts().StrictReturn ||
!MayDropFunctionReturn(getContext(), RetTy) || !MayDropFunctionReturn(getContext(), RetTy) ||
getLangOpts().Sanitize.has(SanitizerKind::Memory) || getLangOpts().Sanitize.has(SanitizerKind::Memory) ||
getLangOpts().Sanitize.has(SanitizerKind::Return); getLangOpts().Sanitize.has(SanitizerKind::Return);
// Enable noundef attribute based on codegen options.
sameerdsUnsubmitted
Not Done
ReplyInline Actions

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".

sameerds: This comment is merely an English translation of the Boolean expression. It should instead…
// Also skip adding the attribute for languages like HIP, CUDA etc.
// which allow uninitialized function arguments. In these languages,
// some functions have multi-threaded semantics and 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.
bool EnableNoundefAttrs = CodeGenOpts.EnableNoundefAttrs &&
(!getLangOpts().allowUninitializedFunctionArgs());
// Determine if the return type could be partially undef // Determine if the return type could be partially undef
if (CodeGenOpts.EnableNoundefAttrs && HasStrictReturn) { if (EnableNoundefAttrs && HasStrictReturn) {
if (!RetTy->isVoidType() && RetAI.getKind() != ABIArgInfo::Indirect && if (!RetTy->isVoidType() && RetAI.getKind() != ABIArgInfo::Indirect &&
DetermineNoUndef(RetTy, getTypes(), DL, RetAI)) DetermineNoUndef(RetTy, getTypes(), DL, RetAI)) {
RetAttrs.addAttribute(llvm::Attribute::NoUndef); RetAttrs.addAttribute(llvm::Attribute::NoUndef);
} }
}
switch (RetAI.getKind()) { switch (RetAI.getKind()) {
case ABIArgInfo::Extend: case ABIArgInfo::Extend:
if (RetAI.isSignExt()) if (RetAI.isSignExt())
RetAttrs.addAttribute(llvm::Attribute::SExt); RetAttrs.addAttribute(llvm::Attribute::SExt);
else else
RetAttrs.addAttribute(llvm::Attribute::ZExt); RetAttrs.addAttribute(llvm::Attribute::ZExt);
LLVM_FALLTHROUGH; LLVM_FALLTHROUGH;
▲ Show 20 Lines • Show All 113 Lines • ▼ Show 20 Lines if (IRFunctionArgs.hasPaddingArg(ArgNo)) {
ArgAttrs[IRFunctionArgs.getPaddingArgNo(ArgNo)] = ArgAttrs[IRFunctionArgs.getPaddingArgNo(ArgNo)] =
llvm::AttributeSet::get( llvm::AttributeSet::get(
getLLVMContext(), getLLVMContext(),
llvm::AttrBuilder(getLLVMContext()).addAttribute(llvm::Attribute::InReg)); llvm::AttrBuilder(getLLVMContext()).addAttribute(llvm::Attribute::InReg));
} }
} }
// Decide whether the argument we're handling could be partially undef // Decide whether the argument we're handling could be partially undef
if (CodeGenOpts.EnableNoundefAttrs && if (EnableNoundefAttrs && DetermineNoUndef(ParamType, getTypes(), DL, AI)) {
DetermineNoUndef(ParamType, getTypes(), DL, AI)) {
Attrs.addAttribute(llvm::Attribute::NoUndef); Attrs.addAttribute(llvm::Attribute::NoUndef);
} }
// 'restrict' -> 'noalias' is done in EmitFunctionProlog when we // 'restrict' -> 'noalias' is done in EmitFunctionProlog when we
// have the corresponding parameter variable. It doesn't make // have the corresponding parameter variable. It doesn't make
// sense to do it here because parameters are so messed up. // sense to do it here because parameters are so messed up.
switch (AI.getKind()) { switch (AI.getKind()) {
case ABIArgInfo::Extend: case ABIArgInfo::Extend:
if (AI.isSignExt()) if (AI.isSignExt())
▲ Show 20 Lines • Show All 3,162 Lines • Show Last 20 Lines

clang/test/CodeGen/PowerPC/ppc64le-varargs-f128.c

clang/test/CodeGenCUDA/address-spaces.cu

clang/test/CodeGenCUDA/builtins-amdgcn.cu

clang/test/CodeGenCUDA/cuda-builtin-vars.cu

clang/test/CodeGenCUDA/kernel-args-alignment.cu

clang/test/CodeGenCUDA/kernel-args.cu

clang/test/CodeGenCUDA/lambda.cu

clang/test/CodeGenCUDA/redux-builtins.cu

clang/test/CodeGenCUDA/surface.cu

clang/test/CodeGenCUDA/unnamed-types.cu

clang/test/CodeGenCUDA/usual-deallocators.cu

clang/test/CodeGenCUDA/vtbl.cu

clang/test/CodeGenCUDASPIRV/kernel-argument.cu

clang/test/CodeGenHIP/hipspv-addr-spaces.cpp

clang/test/CodeGenHIP/noundef-attribute-hip-device-verify.hip

clang/test/CodeGenOpenCL/addr-space-struct-arg.cl

clang/test/CodeGenOpenCL/address-spaces.cl

clang/test/CodeGenOpenCL/amdgcn-automatic-variable.cl

clang/test/CodeGenOpenCL/amdgpu-abi-struct-coerce.cl

clang/test/CodeGenOpenCL/amdgpu-call-kernel.cl

clang/test/CodeGenOpenCL/amdgpu-printf.cl

clang/test/CodeGenOpenCL/as_type.cl

clang/test/CodeGenOpenCL/atomic-ops-libcall.cl

clang/test/CodeGenOpenCL/blocks.cl

clang/test/CodeGenOpenCL/byval.cl

clang/test/CodeGenOpenCL/const-str-array-decay.cl

clang/test/CodeGenOpenCL/constant-addr-space-globals.cl

clang/test/CodeGenOpenCL/convergent.cl

clang/test/CodeGenOpenCL/fpmath.cl

clang/test/CodeGenOpenCL/half.cl

clang/test/CodeGenOpenCL/kernel-param-alignment.cl

clang/test/CodeGenOpenCL/kernels-have-spir-cc-by-default.cl

clang/test/CodeGenOpenCL/no-half.cl

clang/test/CodeGenOpenCL/overload.cl

clang/test/CodeGenOpenCL/size_t.cl

clang/test/CodeGenOpenCL/spir-calling-conv.cl

clang/test/CodeGenOpenCLCXX/address-space-deduction.clcpp

clang/test/CodeGenOpenCLCXX/addrspace-derived-base.clcpp

clang/test/CodeGenOpenCLCXX/addrspace-new-delete.clcpp

clang/test/CodeGenOpenCLCXX/addrspace-of-this.clcpp

clang/test/CodeGenOpenCLCXX/addrspace-operators.clcpp

clang/test/CodeGenOpenCLCXX/addrspace-references.clcpp

clang/test/CodeGenOpenCLCXX/addrspace-with-class.clcpp

clang/test/CodeGenOpenCLCXX/template-address-spaces.clcpp

clang/test/CodeGenSYCL/address-space-conversions.cpp

clang/test/CodeGenSYCL/address-space-mangling.cpp

clang/test/CodeGenSYCL/functionptr-addrspace.cpp

clang/test/CodeGenSYCL/unique_stable_name.cpp

clang/test/OpenMP/amdgcn-attributes.cpp

clang/test/OpenMP/amdgcn_target_global_constructor.cpp

clang/test/OpenMP/assumes_include_nvptx.cpp

clang/test/OpenMP/declare_target_codegen.cpp

clang/test/OpenMP/declare_target_codegen_globalization.cpp

clang/test/OpenMP/declare_target_link_codegen.cpp

clang/test/OpenMP/declare_variant_mixed_codegen.c

clang/test/OpenMP/distribute_codegen.cpp

clang/test/OpenMP/distribute_simd_codegen.cpp

clang/test/OpenMP/nvptx_allocate_codegen.cpp

clang/test/OpenMP/nvptx_data_sharing.cpp

clang/test/OpenMP/nvptx_declare_target_var_ctor_dtor_codegen.cpp

clang/test/OpenMP/nvptx_declare_variant_name_mangling.cpp

clang/test/OpenMP/nvptx_distribute_parallel_generic_mode_codegen.cpp

clang/test/OpenMP/nvptx_lambda_capturing.cpp

clang/test/OpenMP/nvptx_multi_target_parallel_codegen.cpp

clang/test/OpenMP/nvptx_nested_parallel_codegen.cpp

clang/test/OpenMP/nvptx_parallel_codegen.cpp

clang/test/OpenMP/nvptx_parallel_for_codegen.cpp

clang/test/OpenMP/nvptx_target_firstprivate_codegen.cpp

clang/test/OpenMP/nvptx_target_parallel_codegen.cpp

clang/test/OpenMP/nvptx_target_parallel_num_threads_codegen.cpp

clang/test/OpenMP/nvptx_target_parallel_reduction_codegen.cpp

clang/test/OpenMP/nvptx_target_printf_codegen.c

clang/test/OpenMP/nvptx_target_teams_codegen.cpp

clang/test/OpenMP/nvptx_target_teams_distribute_codegen.cpp

clang/test/OpenMP/nvptx_target_teams_distribute_parallel_for_codegen.cpp

clang/test/OpenMP/nvptx_target_teams_distribute_parallel_for_generic_mode_codegen.cpp

clang/test/OpenMP/nvptx_target_teams_distribute_parallel_for_simd_codegen.cpp

clang/test/OpenMP/nvptx_teams_codegen.cpp

clang/test/OpenMP/nvptx_teams_reduction_codegen.cpp

clang/test/OpenMP/nvptx_unsupported_type_codegen.cpp

clang/test/OpenMP/openmp_offload_codegen.cpp

clang/test/OpenMP/reduction_implicit_map.cpp

clang/test/OpenMP/target_firstprivate_codegen.cpp

clang/test/OpenMP/target_parallel_codegen.cpp

clang/test/OpenMP/target_parallel_debug_codegen.cpp

clang/test/OpenMP/target_parallel_for_codegen.cpp

clang/test/OpenMP/target_parallel_for_debug_codegen.cpp

clang/test/OpenMP/target_parallel_for_simd_codegen.cpp

clang/test/OpenMP/target_parallel_if_codegen.cpp

clang/test/OpenMP/target_parallel_num_threads_codegen.cpp

clang/test/OpenMP/target_private_codegen.cpp

clang/test/OpenMP/target_reduction_codegen.cpp

clang/test/OpenMP/target_teams_codegen.cpp

clang/test/OpenMP/target_teams_distribute_codegen.cpp

clang/test/OpenMP/target_teams_distribute_parallel_for_codegen.cpp

clang/test/OpenMP/target_teams_distribute_parallel_for_firstprivate_codegen.cpp

clang/test/OpenMP/target_teams_distribute_parallel_for_private_codegen.cpp

clang/test/OpenMP/target_teams_distribute_parallel_for_simd_codegen.cpp

clang/test/OpenMP/target_teams_distribute_parallel_for_simd_firstprivate_codegen.cpp

clang/test/OpenMP/target_teams_distribute_parallel_for_simd_private_codegen.cpp

clang/test/OpenMP/target_teams_distribute_simd_codegen.cpp

clang/test/OpenMP/target_teams_map_codegen.cpp

clang/test/OpenMP/target_teams_num_teams_codegen.cpp

clang/test/OpenMP/target_teams_thread_limit_codegen.cpp

clang/test/OpenMP/teams_codegen.cpp

llvm/test/Transforms/SimplifyCFG/tautological-conditional-branch-convergent-noundef.ll