bader added inline comments.
================
Comment at: clang/lib/AST/ItaniumMangle.cpp:2379
unsigned TargetAS = Context.getASTContext().getTargetAddressSpace(AS);
- if (TargetAS != 0)
+ if (TargetAS != 0 || Context.getASTContext().getLangOpts().SYCLIsDevice)
ASString = "AS" + llvm::utostr(TargetAS);
----------------
Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > Any reason not to use OpenCL mangling? If you do then you might be
> > > > > able to link against libraries compiled for OpenCL. Also you will get
> > > > > more stable naming i.e. it would not differ from target to target.
> > > > > Any reason not to use OpenCL mangling? If you do then you might be
> > > > > able to link against libraries compiled for OpenCL. Also you will get
> > > > > more stable naming i.e. it would not differ from target to target.
> > > >
> > > > I'm not sure I understand your suggestion. Could you elaborate on
> > > > "OpenCL mangling", please?
> > > >
> > > > Let me clarify the problem this change addresses. The test case
> > > > covering it is located in
> > > > `clang/test/CodeGenSYCL/address-space-parameter-conversions.cpp` lines
> > > > 86-91.
> > > >
> > > > ```
> > > > template <typename T>
> > > > void tmpl(T t) {}
> > > >
> > > > int *NoAS;
> > > > __attribute__((opencl_private)) int *PRIV;
> > > >
> > > > tmpl(PRIV);
> > > > // CHECK-DAG: [[PRIV_LOAD5:%[a-zA-Z0-9]+]] = load i32*, i32*
> > > > addrspace(4)* [[PRIV]].ascast
> > > > // CHECK-DAG: call spir_func void [[PRIV_TMPL:@[a-zA-Z0-9_]+]](i32*
> > > > [[PRIV_LOAD5]])
> > > > tmpl(NoAS);
> > > > // CHECK-DAG: [[NoAS_LOAD5:%[a-zA-Z0-9]+]] = load i32 addrspace(4)*,
> > > > i32 addrspace(4)* addrspace(4)* [[NoAS]].ascast
> > > > // CHECK-DAG: call spir_func void [[GEN_TMPL:@[a-zA-Z0-9_]+]](i32
> > > > addrspace(4)* [[NoAS_LOAD5]])
> > > > ```
> > > > Clang has separate code paths for mangling types w/ and w/o address
> > > > space attributes (i.e. using `Default` address space).
> > > >
> > > > Address space is not mangled if there is no AS attribute (`Default`) or
> > > > if address space attribute is maps to `0` target address space. SPIR
> > > > target maps `*_private` address space to `0`, which causes name
> > > > conflict for the example above.
> > > >
> > > > This change for SYCL compiler enables mangling for non-default address
> > > > space attributes regardless of their mapping to target address space.
> > > It's just that all language address spaces are mangled with the source
> > > spelling in Italium ABI right now, if you check the `else` statement. I
> > > don't think it is part of the official spec yet but it might be better to
> > > stick to the same pattern if possible.
> > > It's just that all language address spaces are mangled with the source
> > > spelling in Italium ABI right now, if you check the `else` statement. I
> > > don't think it is part of the official spec yet but it might be better to
> > > stick to the same pattern if possible.
> >
> > I would really love to avoid changes to the mangler (e.g. to be able to
> > link binaries produced by different front-end like SYCL/OpenCL/CUDA), but I
> > don't know the better way to address the issue
> > Sorry, I don't get what do you suggest here. Could you clarify what exactly
> > I should change, please?
> For now I am just trying to understand why you are not adopting similar
> mangling scheme as for other language address spaces since it gives more
> stable mangling irrespective from the target compiled for.
>
> If you plan to link libraries from other frontends i.e. OpenCL or CUDA the
> mangling you use is different from what they produce. Just have a look at the
> line 2470 that explains OpenCL mangling or line 2494 explaining CUDA
> mangling. FYI similar scheme applies to other language address spaces, so the
> `AS<num>` was only really used for the address spaces that have no source
> spelling i.e. no language semantics.
> For now I am just trying to understand why you are not adopting similar
> mangling scheme as for other language address spaces since it gives more
> stable mangling irrespective from the target compiled for.
According to my understanding this code is used for other language spaces. For
instance, per comments at lines 2455-2457 it's used for OpenCL and CUDA address
spaces.
Do you mean some other mangling scheme?
> If you plan to link libraries from other frontends i.e. OpenCL or CUDA the
> mangling you use is different from what they produce.
SYCL standard doesn't have such functionality. OpenCL C functions are not
mangled (only built-ins), so there should be no problem to link with OpenCL C
libraries.
I know that mangling difference causes some problems for SYCL built-ins
implementations on CUDA, but I don't know all the details. @Naghasan knows
about these. @Naghasan, do you have suggestions to fix the problems caused by
mangling?
> Just have a look at the line 2470 that explains OpenCL mangling or line 2494
> explaining CUDA mangling. FYI similar scheme applies to other language
> address spaces, so the `AS<num>` was only really used for the address spaces
> that have no source spelling i.e. no language semantics.
Is this statement correct? https://godbolt.org/z/n7The38dz - `AS<num>` is used
for OpenCL address spaces mangling when it's compiled for `spir` target.
================
Comment at: clang/lib/Basic/Targets/SPIR.h:129
+ TargetInfo::adjust(Opts);
+ setAddressSpaceMap(/*DefaultIsGeneric=*/Opts.SYCLIsDevice);
+ }
----------------
Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > Anastasia wrote:
> > > > Ok, do you still plan to add a `FIXME` as mentioned previously
> > > > explaining why we need to reset the map here?
> > > Btw I was just thinking of another alternative here.
> > >
> > > What do you think about just setting a value in `Default` AS entry then
> > > we wouldn't need any extra map at all in this case? So something like:
> > >
> > >
> > > ```
> > > AddrSpaceMap[LangAS::Default] = AddrSpaceMap[LangAS::opencl_generic];
> > > ```
> > > with a good explanation in `FIXME`? :)
> > >
> > > Btw I was just thinking of another alternative here.
> > >
> > > What do you think about just setting a value in `Default` AS entry then
> > > we wouldn't need any extra map at all in this case? So something like:
> > >
> > >
> > > ```
> > > AddrSpaceMap[LangAS::Default] = AddrSpaceMap[LangAS::opencl_generic];
> > > ```
> > > with a good explanation in `FIXME`? :)
> > >
> >
> > I think this won't work because AddrSpaceMap is a constant.
> > Regarding `FIXME`, could you point where it was previously mentioned,
> > please? I think it might miss it. I can add a link to the SYCL spec -
> > https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:genericAddressSpace.
> > Will it be sufficient?
> I mean the FIXME is about the issue with Clang design:
> https://reviews.llvm.org/D89909#inline-941733
>
> Basically, just explain why we need to reset the map and that it is only
> needed for `Default` address space.
Added FIXME comment.
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985
+ "Address space agnostic languages only");
+ LangAS DefaultGlobalAS = getLangASFromTargetAS(
+ CGM.getContext().getTargetAddressSpace(LangAS::sycl_global));
----------------
Anastasia wrote:
> bader wrote:
> > Anastasia wrote:
> > > bader wrote:
> > > > Anastasia wrote:
> > > > > Since you are using SYCL address space you should probably guard this
> > > > > line by SYCL mode... Btw the same seems to apply to the code below
> > > > > as it implements SYCL sematics?
> > > > >
> > > > > Can you add spec references here too.
> > > > >
> > > > > Also there seems to be nothing target specific in the code here as
> > > > > you are implementing what is specified by the language semantics.
> > > > > Should this not be moved to `GetGlobalVarAddressSpace` along with the
> > > > > other language handling?
> > > > >
> > > > > I am not very familiar with this part of address space handling
> > > > > though. I would be more comfortable if @rjmccall could take a look
> > > > > too.
> > > > This code assigns target address space "global variables w/o address
> > > > space attribute".
> > > > SYCL says it's "implementation defined" (from
> > > > https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#subsec:commonAddressSpace):
> > > >
> > > > > Namespace scope
> > > > > If the type is const, the address space the declaration is assigned
> > > > > to is implementation-defined. If the target of the SYCL backend can
> > > > > represent the generic address space, then the assigned address space
> > > > > must be compatible with the generic address space.
> > > > > Namespace scope non-const declarations cannot be used within a
> > > > > kernel, as restricted in Section 5.4. This means that non-const
> > > > > global variables cannot be accessed by any device kernel or code
> > > > > called by the device kernel.
> > > >
> > > > I added clarification that SPIR target allocates global variables in
> > > > global address space to https://reviews.llvm.org/D99488 (see line #248).
> > > >
> > > > @rjmccall, mentioned in the mailing list discussion that this callbacks
> > > > were developed for compiling C++ to AMDGPU target, so this not
> > > > necessary designed only for SYCL, but it works for SYCL as well.
> > > After all what objects are allowed to bind to non-default address space
> > > here is defined in SYCL spec even if the exact address spaces are not
> > > defined so it is not completely a target-specific behavior.
> > >
> > > My understanding of the API you are extending (judging from its use) is
> > > that it allows you to extend the language sematics with some
> > > target-specific setup. I.e. you could add extra address spaces to C++ or
> > > OpenCL or any other language. But here you are setting the language
> > > address spaces instead that are mapped to the target at some point
> > > implicitly.
> > >
> > > It seems like this change better fits to
> > > `CodeGenModule::GetGlobalVarAddressSpace` that already contains very
> > > similar logic?
> > >
> > > Otherwise, it makes more sense to use target address spaces directly
> > > instead of SYCL language address spaces. But either way, we should guard
> > > it by SYCL mode somehow as we have not established this as a universal
> > > logic for SPIR.
> > > It seems like this change better fits to
> > > `CodeGenModule::GetGlobalVarAddressSpace` that already contains very
> > > similar logic?
> >
> > This was the original implementation (see
> > https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use
> > this callback instead.
> > Both ways work for me, but the implementation proposed by John is easier to
> > maintain.
> >
> > > Otherwise, it makes more sense to use target address spaces directly
> > > instead of SYCL language address spaces. But either way, we should guard
> > > it by SYCL mode somehow as we have not established this as a universal
> > > logic for SPIR.
> >
> > I've updated the code to use target address space. I also added an
> > assertion for SYCL language mode, although I think SPIR doesn't support
> > global variables in address spaces other than global or constant regardless
> > of the language mode, so I think the logic is universal.
> > This was the original implementation (see
> > https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use
> > this callback instead.
>
> Did you mean to link some particular conversation? Currently, it resets to
> the top of the review.
>
> > Both ways work for me, but the implementation proposed by John is easier
> > to maintain.
>
> I can't see why the same code would be harder to maintain in the caller. If
> anything it should reduce the maintenance because the same logic won't need
> to be implemented by every target.
>
> > I also added an assertion for SYCL language mode, although I think SPIR
> > doesn't support global variables in address spaces other than global or
> > constant regardless of the language mode, so I think the logic is universal.
>
> Asserts don't guard this logic to be applied universally. And since the IR
> was generated like this for about 10 years I don't feel comfortable about
> just changing it silently.
>
> To my memory SPIR spec never put restrictions to the address spaces. It only
> described the generation for OpenCL C. So if you compile from C you would
> have everything in the default address space. And even OpenCL rules doesn't
> seem to be quite accurate in your patch as in OpenCL C globals can be in
> `__global`, `__constant` or `__local`. However, the SPIR spec was
> discontinued quite a while ago and the implementation of SPIR has evolved so
> I am not sure how relevant the spec is now.
>
> Personally, I feel the behavior you are implementing is governed by the
> language soI think it is more logical to encapsulate it to avoid interfering
> with other language modes.
>
> > This was the original implementation (see
> > https://reviews.llvm.org/D89909?id=299795), but @rjmccall suggested to use
> > this callback instead.
>
> Did you mean to link some particular conversation? Currently, it resets to
> the top of the review.
I pointed to the patch version implementing address space deduction in
`CodeGenModule::GetGlobalVarAddressSpace`.
Conversion pointer is RFC in the mailing list:
http://clang-developers.42468.n3.nabble.com/RFC-Re-use-OpenCL-address-space-attributes-for-SYCL-tp4068754p4069039.html
> > Both ways work for me, but the implementation proposed by John is easier
> > to maintain.
>
> I can't see why the same code would be harder to maintain in the caller. If
> anything it should reduce the maintenance because the same logic won't need
> to be implemented by every target.
>
> > I also added an assertion for SYCL language mode, although I think SPIR
> > doesn't support global variables in address spaces other than global or
> > constant regardless of the language mode, so I think the logic is universal.
>
> Asserts don't guard this logic to be applied universally. And since the IR
> was generated like this for about 10 years I don't feel comfortable about
> just changing it silently.
>
> To my memory SPIR spec never put restrictions to the address spaces. It only
> described the generation for OpenCL C. So if you compile from C you would
> have everything in the default address space. And even OpenCL rules doesn't
> seem to be quite accurate in your patch as in OpenCL C globals can be in
> `__global`, `__constant` or `__local`. However, the SPIR spec was
> discontinued quite a while ago and the implementation of SPIR has evolved so
> I am not sure how relevant the spec is now.
>
> Personally, I feel the behavior you are implementing is governed by the
> language soI think it is more logical to encapsulate it to avoid interfering
> with other language modes.
>
Added early exist for non-SYCL modes.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89909/new/
https://reviews.llvm.org/D89909
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
