r314872 - We allow implicit function declarations as an extension in all C dialects. Remove OpenCL special case.
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Wed Aug 22 13:16:36 PDT 2018
On Wed, 22 Aug 2018 at 06:55, Anastasia Stulova via cfe-commits <
cfe-commits at lists.llvm.org> wrote:
> Hi Richard,
>
> > This is incorrect. Implicit function declarations declare unprototyped
> functions, which are *not* variadic, and are in fact supported by Clang's
> OpenCL language mode.
>
> We have removed the support for the unprototyped functions from the OpenCL
> as well. See commit: https://reviews.llvm.org/D33681. This is the reason
> why in the OpenCL mode we now generated empty parameter list instead of
> unprototyped function like for C in the examples I provided before (that is
> not governed now by any standard or any compiler extension).
>
That's interesting. Do I understand from that review thread that we're
diverging from the OpenCL specification in treating () as (void) rather
than as an unprototyped function declaration? If so, that seems like a
surprising and concerning decision, unless we're confident that the OpenCL
language really did mean to change this aspect of the C semantics and
omitted the wording change by oversight. (And I've checked, and can find
nothing in the OpenCL specification that justifies this: it looks like a
Clang bug that we reject
int f();
void g() { f(1, 2, 3); }
int f(int a, int b, int c) { return 0; }
... for example, unless Khronos really did mean to use the C++ rule.)
If it is indeed the intent of the OpenCL specification to treat () as
(void) like in C++ and not have unprototyped functions, then I think it
does make sense to also disable our implicitly declared functions extension
in OpenCL. But, conversely, if the OpenCL specification instead intends to
follow C here and allow unprototyped functions, then it is a bug in our
OpenCL support that we don't follow that intent, and when that bug is fixed
it makes sense to continue to accept implicit function declarations as an
extension.
> I would have sympathy for your position if we did not produce an
> extension warning on this construct by default. But we do, and it says the
> construct is invalid in OpenCL; moreover, in our strict conformance mode
> (-pedantic-errors), we reject the code.
>
> I understand the motivation for C to maintain the compatibility with the
> previous standards and other compilers (like gcc) to be able to support the
> legacy code. However, for OpenCL we don't have this requirement wrt older C
> standards. And therefore it is desirable to take advantage of this and
> remove problematic features that are generally confusing for developers or
> that can't be efficiently supported by the targets (especially if there is
> a little cost to that).
>
For this "can't be efficiently supported by the target" claim, I think
you're conflating the target and the language mode. If some target can't
reasonably support variadic functions, we should disable variadic functions
for that target. That has *nothing* to do with the language mode; targets
and language modes can be combined arbitrarily. [If I want to use Clang to
compile OpenCL code for my PDP-11, then I should be allowed to do that
(assuming I have a suitable LLVM backend), and that target presumably would
support variadic functions just fine.] Likewise, if the target doesn't
support variadic functions, we should not be generating variadic function
types when producing IR (particularly in calls to non-variadic functions
like in your example elsewhere in this thread). This is true regardless of
whether the source language is OpenCL or C89 or C++ or anything else.
It is a goal of Clang to allow its various features to be used together,
including combining them in ways that we didn't think of. The particular
case of implicit function declarations is not especially important in and
of itself, but the underlying principle is: the OpenCL language mode of
Clang should not disable other Clang extensions unless there's some
fundamental reason why they are incompatible.
Consider this: we allow implicit function declarations in languages based
on C in order to allow C89 code (or code that started as C89 code) to be
built unchanged in those languages. That applies to OpenCL and Objective-C
as much as it applies to C99 and C11. (It doesn't apply to C++ because
there is no such thing as an unprototyped function in C++'s type system.)
________________________________
> From: Richard Smith <richard at metafoo.co.uk>
> Sent: 21 August 2018 22:09:35
> To: Anastasia Stulova
> Cc: cfe-commits; nd
> Subject: Re: r314872 - We allow implicit function declarations as an
> extension in all C dialects. Remove OpenCL special case.
>
> On Tue, 21 Aug 2018 at 07:41, Anastasia Stulova via cfe-commits <
> cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>> wrote:
>
> If there are no objections I would like to revert this old commit that
> coverts error about implicit function declaration into a warning.
>
>
> We have decided to generate an error for this
> https://reviews.llvm.org/D31745 because for OpenCL variadic prototypes
> are disallowed (section 6.9.e,
> https://www.khronos.org/registry/OpenCL/specs/opencl-2.0-openclc.pdf) and
> the implicit prototype requires variadic support.
>
> This is incorrect. Implicit function declarations declare unprototyped
> functions, which are *not* variadic, and are in fact supported by Clang's
> OpenCL language mode.
>
> See C90 6.5.4.3 Semantics, last paragraph, and 6.3.2.2 Semantics, second
> paragraph.
>
> So that argument does not appear to apply. The reason we accept
> implicitly-declared functions outside of our C89 mode is because this is an
> explicit, supported Clang extension. Generally, Clang intends to support
> using all of its extensions together, unless there is some fundamental
> reason why they cannot be combined. So, just as it doesn't make sense for
> our OpenCL language mode to conflict with, say, AltiVec vector extensions,
> it doesn't make sense for the OpenCL language mode to conflict with our
> implicitly-declared functions extension.
>
> I would have sympathy for your position if we did not produce an extension
> warning on this construct by default. But we do, and it says the construct
> is invalid in OpenCL; moreover, in our strict conformance mode
> (-pedantic-errors), we reject the code.
>
> As most vendors that support OpenCL don't support variadic functions it
> was decided to restrict this explicitly in the spec (section s6.9.u). There
> is a little bit of more history in https://reviews.llvm.org/D17438.
>
>
> Currently the code that can't run correctly on most OpenCL targets
> compiles successfully. The problem can't be easily seen by the OpenCL
> developers since it's not very common to retrieve the compilation warning
> log during online compilation. Also generated IR doesn't seem to be correct
> if I compare with the similar code in C.
>
> Example:
> 1 typedef long long16 __attribute__((ext_vector_type(16)));
> 2 void test_somefunc( __global int *d, __global void *s )
> 3 {
> 4 int i = get_global_id(0);
> 5 d[i] = somefunc((( __global long16 *)s)[i]);
> 6 }
>
> Is generated to:
>
> %call1 = call i32 (<16 x i64>*, ...) bitcast (i32 ()* @somefunc to i32
> (<16 x i64>*, ...)*)(<16 x i64>* byval nonnull align 128
> %indirect-arg-temp) #2
> ...
>
> declare i32 @somefunc() local_unnamed_addr #1
>
> Equivalent C code at least generates variadic function prototype correctly:
>
> %call1 = call i32 (<16 x i64>*, ...) bitcast (i32 (...)* @somefunc to i32
> (<16 x i64>*, ...)*)(<16 x i64>* byval align 128 %indirect-arg-temp)
> ...
> declare i32 @somefunc(...)
>
> Anastasia
> ________________________________
> From: cfe-commits <cfe-commits-bounces at lists.llvm.org<mailto:
> cfe-commits-bounces at lists.llvm.org>> on behalf of Richard Smith via
> cfe-commits <cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org
> >>
> Sent: 04 October 2017 02:58
> To: cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>
> Subject: r314872 - We allow implicit function declarations as an extension
> in all C dialects. Remove OpenCL special case.
>
> Author: rsmith
> Date: Tue Oct 3 18:58:22 2017
> New Revision: 314872
>
> URL: http://llvm.org/viewvc/llvm-project?rev=314872&view=rev
> Log:
> We allow implicit function declarations as an extension in all C dialects.
> Remove OpenCL special case.
>
> Modified:
> cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> cfe/trunk/lib/Sema/SemaDecl.cpp
> cfe/trunk/test/SemaOpenCL/clang-builtin-version.cl<
> http://clang-builtin-version.cl>
> cfe/trunk/test/SemaOpenCL/to_addr_builtin.cl<http://to_addr_builtin.cl
> >
>
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=314872&r1=314871&r2=314872&view=diff
>
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Tue Oct 3
> 18:58:22 2017
> @@ -355,7 +355,7 @@ def warn_implicit_function_decl : Warnin
> "implicit declaration of function %0">,
> InGroup<ImplicitFunctionDeclare>, DefaultIgnore;
> def ext_implicit_function_decl : ExtWarn<
> - "implicit declaration of function %0 is invalid in C99">,
> + "implicit declaration of function %0 is invalid in
> %select{C99|OpenCL}1">,
> InGroup<ImplicitFunctionDeclare>;
> def note_function_suggestion : Note<"did you mean %0?">;
>
> @@ -8449,8 +8449,6 @@ def err_opencl_scalar_type_rank_greater_
> "element. (%0 and %1)">;
> def err_bad_kernel_param_type : Error<
> "%0 cannot be used as the type of a kernel parameter">;
> -def err_opencl_implicit_function_decl : Error<
> - "implicit declaration of function %0 is invalid in OpenCL">;
> def err_record_with_pointers_kernel_param : Error<
> "%select{struct|union}0 kernel parameters may not contain pointers">;
> def note_within_field_of_type : Note<
>
> Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=314872&r1=314871&r2=314872&view=diff
>
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaDecl.cpp Tue Oct 3 18:58:22 2017
> @@ -12642,17 +12642,15 @@ NamedDecl *Sema::ImplicitlyDefineFunctio
> }
>
> // Extension in C99. Legal in C90, but warn about it.
> + // OpenCL v2.0 s6.9.u - Implicit function declaration is not supported.
> unsigned diag_id;
> if (II.getName().startswith("__builtin_"))
> diag_id = diag::warn_builtin_unknown;
> - // OpenCL v2.0 s6.9.u - Implicit function declaration is not supported.
> - else if (getLangOpts().OpenCL)
> - diag_id = diag::err_opencl_implicit_function_decl;
> - else if (getLangOpts().C99)
> + else if (getLangOpts().C99 || getLangOpts().OpenCL)
> diag_id = diag::ext_implicit_function_decl;
> else
> diag_id = diag::warn_implicit_function_decl;
> - Diag(Loc, diag_id) << &II;
> + Diag(Loc, diag_id) << &II << getLangOpts().OpenCL;
>
> // If we found a prior declaration of this function, don't bother
> building
> // another one. We've already pushed that one into scope, so there's
> nothing
>
> Modified: cfe/trunk/test/SemaOpenCL/clang-builtin-version.cl<
> http://clang-builtin-version.cl>
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaOpenCL/clang-builtin-version.cl?rev=314872&r1=314871&r2=314872&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/SemaOpenCL/clang-builtin-version.cl<
> http://clang-builtin-version.cl> (original)
> +++ cfe/trunk/test/SemaOpenCL/clang-builtin-version.cl<
> http://clang-builtin-version.cl> Tue Oct 3 18:58:22 2017
> @@ -1,4 +1,4 @@
> -// RUN: %clang_cc1 %s -fblocks -verify -pedantic -fsyntax-only
> -ferror-limit 100
> +// RUN: %clang_cc1 %s -fblocks -verify -pedantic-errors -fsyntax-only
> -ferror-limit 100
>
> // Confirm CL2.0 Clang builtins are not available in earlier versions
>
>
> Modified: cfe/trunk/test/SemaOpenCL/to_addr_builtin.cl<
> http://to_addr_builtin.cl>
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaOpenCL/to_addr_builtin.cl?rev=314872&r1=314871&r2=314872&view=diff
>
> ==============================================================================
> --- cfe/trunk/test/SemaOpenCL/to_addr_builtin.cl<http://to_addr_builtin.cl>
> (original)
> +++ cfe/trunk/test/SemaOpenCL/to_addr_builtin.cl<http://to_addr_builtin.cl>
> Tue Oct 3 18:58:22 2017
> @@ -10,7 +10,7 @@ void test(void) {
>
> glob = to_global(glob, loc);
> #if __OPENCL_C_VERSION__ < CL_VERSION_2_0
> - // expected-error at -2{{implicit declaration of function 'to_global' is
> invalid in OpenCL}}
> + // expected-warning at -2{{implicit declaration of function 'to_global'
> is invalid in OpenCL}}
> // expected-warning at -3{{incompatible integer to pointer conversion
> assigning to '__global int *' from 'int'}}
> #else
> // expected-error at -5{{invalid number of arguments to function:
> 'to_global'}}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org<mailto:cfe-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20180822/1d19f4cb/attachment-0001.html>
More information about the cfe-commits
mailing list