warn not error when SYCL_EXTERNAL applied to anonymous namespaces

[jeffhammond]

LLNL Quicksilver (https://github.com/LLNL/Quicksilver) uses anonymous namespaces extensively for code that is only used within a single compilation unit, which is the modern C++ equivalent of static functions (https://stackoverflow.com/questions/154469/unnamed-anonymous-namespaces-vs-static-functions).

Given that anonymous namespaces only need to be resolved in a single compilation unit, I do not see why the compiler can't support these. We should support as a SYCL extension ASAP and propose to Khronos.

I just spent the last hour fixes ~20 instances of this warning.

error: 'sycl_device' attribute cannot be applied to a static function or function in an anonymous namespace

I don't think you need a reproducer, but https://github.com/jeffhammond/Quicksilver/tree/29f799ed09d16145aa63753d7e8527946a901f6d has a non-minimal one.

[AlexeySachkov]

@jeffhammond, could you please elaborate, why do you need to have SYCL_EXTERNAL at such functions?

From SYCL 1.2.1 spec, section 6.9.1 SYCL Functions and methods linkage:

The default behavior in SYCL applications is that all the definitions and declarations of the functions and methods are available to the SYCL compiler, in the same translation unit. When this is not the case, all the symbols that need to be exported to a SYCL library or from a C++ library to a SYCL application need to be defined using the macro: SYCL_EXTERNAL.

If you have function in anonymous namespace, you cannot call it from other translation units anyway and it is unclear to me why do you need SYCL_EXTERNAL if its intent is to say that this function will be called from another translation unit (or that the function is defined in another translation unit).

Am I missing something? BTW, looking at the reproducer, I cannot find any SYCL_EXTERNAL there, but probably GitHub search just doesn't work well and I have to use grep locally

[jeffhammond]

@AlexeySachkov I'll make a reproducer. The commit after the one I shared added SYCL_EXTERNAL everywhere required and the necessary workarounds.

[jeffhammond]

Basically, take https://github.com/jeffhammond/Quicksilver/blob/ae353aa8d178f2eef8f2c13ee4923633aba1d51b/src/dpct/MCT.cc and change namespace NO back to namespace and NO:: back to nothing.

[jeffhammond]

MCVE

$ dpcpp -std=c++14 -O3 -ferror-limit=1 -c bug.cc 
bug.cc:5:5: error: 'sycl_device' attribute cannot be applied to a static function or function in an anonymous namespace
    SYCL_EXTERNAL
    ^
<built-in>:735:38: note: expanded from here
#define SYCL_EXTERNAL __attribute__((sycl_device))
                                     ^
1 error generated.

bug.cc

#include <CL/sycl.hpp>

namespace
{
    SYCL_EXTERNAL
    int foo(int i) { return i*i; }
}

namespace Bar
{
    SYCL_EXTERNAL
    int bar(int i) { return foo(i) + 1; }
}

[jeffhammond]

The reason why this matters is that, when we use the DPCT, code labeled as device code will have the SYCL_EXTERNAL attributed on it. It's annoying to have to manually change this. Given that the compiler knows from C++ that code in an anonymous namespace doesn't have to be exported, it should ignore or at least not invoke a fatal error on this usage.

Relaxing the parser here is similar to how we do not require kernel names. It's a user convenience feature. I do not mine at all if it requires a flag like -fsycl-allow-sycl-external-on-anonymous-namespaces.

[bader]

I guess compiler can just ignore SYCL_EXTERNAL applied to foo.

[jeffhammond]

That would be nice. Thanks!

And I don't mind a warning along the lines of warning: 'sycl_device' attribute applied to a static function or function in an anonymous namespace has no effect.

[Pennycook]

The reason why this matters is that, when we use the DPCT, code labeled as device code will have the SYCL_EXTERNAL attributed on it. It's annoying to have to manually change this. Given that the compiler knows from C++ that code in an anonymous namespace doesn't have to be exported, it should ignore or at least not invoke a fatal error on this usage.

Couldn't this also be fixed in DPCT? If the function declared as device code is static or in an anonymous namespace, arguably DPCT shouldn't be marking it as SYCL_EXTERNAL in the first place.

[jeffhammond]

I'm not entirely sure that the DPCT added SYCL_EXTERNAL in the wrong place, but Quicksilver has macros for GPU code sections and they don't necessarily match the SYCL specification precisely (they work for CUDA and OpenMP target) and I got this problem as I was modifying the DPCT version so that it would work properly.

If I am forced to fix this manually, I'll have to find every single instance of HOST_DEVICE in Quicksilver and determine whether it is a SYCL_EXTERNAL or not, and then refactor the entire macro infrastructure so that Quicksilver can compile to OpenMP host, OpenMP target, CUDA and SYCL. This O(N) effort becomes especially prohibitive in a real code that is like Quicksilver, but 10-100x larger.

[Pennycook]

I understand why this is frustrating, but I still think we should pay attention to how SYCL_EXTERNAL is defined in the specification before we change our compiler's handling of it -- especially if there's any interest in Quicksilver being able to compile with different SYCL compilers.

Marking every function as SYCL_EXTERNAL when it isn't necessary is potentially dangerous and may confuse developers.

For example, SYCL 1.2.1 places restrictions on the definition of a SYCL_EXTERNAL function: "The function cannot use raw pointers as parameter or return types. Explicit pointer classes must be used
instead." It's also worth noting that SYCL_EXTERNAL is a macro with an implementation-defined value; applying SYCL_EXTERNAL to a static function would result in a compiler error if that macro is defined simply as extern (which would be a very reasonable implementation for an ahead-of-time or host C++ compiler).

[jeffhammond]

The reason to relax the compiler check in this particular case is that C++ already says that SYCL_EXTERNAL can't have any effect.

Will the USM proposal relax the restriction on SYCL_EXTERNAL to allow raw pointers as parameter or return types?

[Pennycook]

The reason to relax the compiler check in this particular case is that C++ already says that SYCL_EXTERNAL can't have any effect.

Sure. I'm just worried that SYCL_EXTERN might expand to something that's incompatible with being inside an anonymous namespace. If that can't happen, I'll object less.

Will the USM proposal relax the restriction on SYCL_EXTERNAL to allow raw pointers as parameter or return types?

No, this is unrelated to USM. The issue here is that SYCL 1.2.1 address space inference rules can't work across a function call boundary. When an external function is compiled, it needs to know which address space its arguments are in. It can't be inferred from the callsite (because it's in a different translation unit).

[AlexeySachkov]

@jeffhammond, thanks for the background of the issue

And I don't mind a warning along the lines of warning: 'sycl_device' attribute applied to a static function or function in an anonymous namespace has no effect.

I would vote for warning too: someone who just placed SYCL_EXTERNAL all over the place (by whatever reason) will still have successful compilation and AFAIK, we have something like -fsycl-strict, which could turn this warning into an error for those who wants to place the macro correctly.

Marking every function as SYCL_EXTERNAL when it isn't necessary is potentially dangerous and may confuse developers.

I've just realized one more thing: this macro is optional and only defined by the SYCL implementation if it supports offline linking. But I guess it only matters if some portability is desired and from portability point of view it is probably better to avoid using this macro at all (or be ready to workaround SYCL spec limitations somehow)

[keryell]

Actually it is possible to have extern in anonymous namespace, so it makes sense to allow SYCL_EXTERNAL with a similar no-op behaviour.
https://en.cppreference.com/w/cpp/language/namespace

Useful for metaprogramming or recycling old code like the case discussed here.

Can someone create an internal spec issues to see if that makes sense before doing a 1.2.1 PR?

[Pennycook]

Actually it is possible to have extern in anonymous namespace, so it makes sense to allow SYCL_EXTERNAL with a similar no-op behaviour.

Interesting. I didn't know about this -- thanks for pointing it out! I agree that it makes sense for SYCL_EXTERNAL to behave the same way as extern in standard C++ in this case.