Anastasia 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);
----------------
bader wrote:
> 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.
> 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.
I am very confident this is correct https://godbolt.org/z/8o6vdYEPc though
perhaps comments could be made more clear and feel free to improve them if you
find them misleading.
So if you follow the flow here you can see that if the target maps the address
space to a distinct non-zero value then it is using the target scheme mangling
otherwise it will use the language spelling.
I believe this is done to prevent the name clashing, so if let's say you have
overloaded functions:
```
foo(__global int*){}
foo(__local int*){}
```
and your target doesn't have the target address spaces (i.e. you would end up
with `AS0` and `AS0` in both) then you would want to mangle the functions
differently to avoid generating two definitions with the same name?
I think though this flow still has a flaw for the cases the target would map
language address spaces to non-distinct and non-zero values there could still
be a clash. Therefore I think using source address spaces would have been more
stable but it would reduce the ability to link functions produced from
different language modes.
================
Comment at: clang/lib/Basic/Targets/SPIR.h:131
+ TargetInfo::adjust(Opts);
+ // FIXME: SYCL specification considers unannotated pointers and references
+ // to be pointing to the generic address space. See section 5.9.3 of
----------------
Ok, let's be a bit more specific regarding the clang design flaw i.e. I would
add something like
```
Currently, there is no way of representing SYCL's default address space
language semantic along with the semantics of embedded C's default address
space in the same address space map. Hence the map needs to be reset to allow
mapping to the desired value of 'Default' entry for SYCL.
```
You could also mention that SYCL has the same semantics as CUDA which would
hint that this logic should be generalised to multiple languages not just SYCL.
================
Comment at: clang/lib/Basic/Targets/SPIR.h:140
+ // space must be compatible with the generic address space
+ return LangAS::sycl_global;
+ }
----------------
This needs a language guard too?
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10081
+
+ assert(AddrSpace == LangAS::Default || isTargetAddressSpace(AddrSpace));
+ if (AddrSpace != LangAS::Default)
----------------
Can you add an error message string to the assert?
Also should you just assert that that address space is 0 i.e. not specified
yet? Otherwise I don't think you should override it. Although you actually
handle this case below as a valid input? Do you actually need this assert then?
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:10086
+ if (CGM.isTypeConstant(D->getType(), false)) {
+ if (auto ConstAS = CGM.getTarget().getConstantAddressSpace())
+ return *ConstAS;
----------------
Do you need to query the target from `CGM`? It returns `TargetInfo` which
member you are overriding here.
https://clang.llvm.org/doxygen/classclang_1_1CodeGen_1_1CodeGenModule.html#a09a815215ff67d2e52b7605af35c40cb
However, I would just change this code to avoid generalizing since the
generalization is SYCL specific anyway. The SPIR target does have a constant
address space segment, so I would find it strange that it maps to global as a
general rule. Why not to make it simple and just return global address space
for SYCL in all cases?
================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:9985
+ "Address space agnostic languages only");
+ LangAS DefaultGlobalAS = getLangASFromTargetAS(
+ CGM.getContext().getTargetAddressSpace(LangAS::sycl_global));
----------------
bader wrote:
> 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.
> 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
>
This looks actually very neat and I can't see that anyone had any concerns
about this.
I think John's comment on RFC is to point out that there are also Target hooks
available should you need to override the language semantics but there is no
statement that you should prefer it instead of implementing the language rules.
I think the language semantics should take precedence.
> Added early exist for non-SYCL modes.
To improve the understanding I would prefer if you guard the logic with if
statement and return the original address space as default right at the end:
```
if (CGM.getLangOpts().SYCLIsDevice) {
// do what you need for SYCL
}
// default case - just return original address space
return AddrSpace;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D89909/new/
https://reviews.llvm.org/D89909
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
