Page MenuHomePhabricator

Add MS Mangling for OpenCL Pipe types, add mangling test.
ClosedPublic

Authored by erichkeane on Mar 5 2020, 8:18 AM.

Details

Summary

SPIRV2.0 Spec only specifies Linux mangling, however our downstream has
use for a Windows mangling for these types.

Unfortunately, the SPIRV
spec specifies a single mangling for all pipe types, despite clang
allowing overloading on these types. Because of this, this patch
chooses to mangle the read/writability and element type for the windows
mangling.

The windows manglings in the test all demangle according to demangler:
"void cdecl test1(struct clang::ocl_pipe<int,1>)
"void cdecl test2(struct clang::ocl_pipe<float,0>)
"void cdecl test2(struct clang::ocl_pipe<int,1>)
"void cdecl test3(struct clang::ocl_pipe<int const,1>)
"void cdecl test4(struct clang::ocl_pipe<union
clang::vector<unsigned char,3>,1>)
"void cdecl test5(struct clang::ocl_pipe<union
clang::vector<int,4>,1>)
"void cdecl test_reserved_read_pipe(struct clang::_ASCLglobal<struc
Person > * ptr64,struct clang::ocl_pipe<struct Person,1>)

Diff Detail

Event Timeline

erichkeane created this revision.Mar 5 2020, 8:18 AM

Ping! @Anastasia I was hoping for your input on this, since it is OpenCLC++.

Anastasia added inline comments.Mar 10 2020, 5:08 AM
clang/lib/AST/MicrosoftMangle.cpp
2953

We don't seem to add namespace for other OpenCL types, although I am not against it as I find it actually cleaner.

Since the mangling deviates what is documented can you add some comments here explaining your mangling scheme?

clang/test/CodeGenOpenCLCXX/pipe_types_mangling.cl
2 ↗(On Diff #248484)

Does this work for OpenCL C (although you would need to add overloading attribute)? If so maybe worth adding a RUN line too.

If it works for OpenCL C I would move this into test/CodeGenOpenCL. In this folder we only keep what is C++ specific. Although overloading is technically C++ but we use it in C too.

20 ↗(On Diff #248484)

any reason this is different from the rest?

erichkeane marked 3 inline comments as done.Mar 10 2020, 7:12 AM
erichkeane added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
2953

The Microsoft mangling scheme is owned by the Microsoft Corporation, so adding something to their mangling (like 8ocl_pipe) isn't permitted. Thus, the clang project mangles our types that aren't supported by Microsoft as a type in the __clang namespace.

You'll note that we do it for a bunch of types above, including AddressSpaceType, VectorType, _Complex, _Float16, _Half, etc.

clang/test/CodeGenOpenCLCXX/pipe_types_mangling.cl
20 ↗(On Diff #248484)

Sorry, I don't understand the question perhaps? (different in what way?).

Do you mean why it is in a macro? I wanted an example to show that overloading works so I chose test2. As the comment says, the OpenCL standard mangling doesn't support overloading on just pipes (since they don't take element type and read/write into account).

In those cases, we get a code-gen emitted diagnostic that two functions have identical mangling.

Thanks @Anastasia for the review! Comments inline.

Moved the test as requested, and added OpenCL tests for both OSes.

Ping! Anyone else have feedback?

bader added a comment.Mar 12 2020, 6:37 AM

LGTM. Thanks!

Anastasia added inline comments.Mar 12 2020, 6:50 AM
clang/lib/AST/MicrosoftMangle.cpp
2953

Are you saying that we have a bug for the other OpenCL type i.e. images, samplers, etc?

clang/test/CodeGenOpenCL/pipe_types_mangling.cl
13

Any reason to test unmangled?

26

I am still unclear why is this special case?

erichkeane marked 3 inline comments as done.Mar 12 2020, 7:13 AM
erichkeane added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
2953

I don't know those well enough to answer, but at least in this case 'Pipe' isn't a Type. It is a Type Class. Because of this, having a single mangling for it is incorrect. It appears images likely is going to have this problem as well.

clang/test/CodeGenOpenCL/pipe_types_mangling.cl
13

Because you asked to validate the OpenCL cases as well.

26

It isn't possible to overload on these types in Linux mode, because the OpenCL spec on ItaniumABI has a specific mangling that doesn't take element type and read/write into effect. The probelm is that on Linux BOTH versions of 'test2' end up with the mangling '@_Z5test28ocl_pipe' (and we get an error in codegen).

erichkeane marked an inline comment as done.Mar 12 2020, 7:34 AM
erichkeane added inline comments.
clang/lib/AST/MicrosoftMangle.cpp
2953

I've looked closer, and I don't believe this is a problem for other openCL types (other than they are not reserved names). In ~2090 in this file those are implemented as a struct named ocl_*, which while not reserved in C++ will demangle. Image types are mangled differently, also as a struct type, so i think those are OK.

These pipe types aren't mangled separately in Linux (hence why overloading doesn't work with them in Linux).

Ping! I would expect this to be non-controversial, but if there is a different reviewer anyone can suggest, I'd appreciate it!

Thanks!

Does anyone have any comments for me? This is blocking using pipes on windows targets.

This revision is now accepted and ready to land.Mar 25 2020, 7:12 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2020, 8:05 AM

Sorry for the delayed comments. It would be good to address those in a separate commit if possible.

clang/test/CodeGenOpenCL/pipe_types_mangling.cl
13

Ok, I guess we should add overloadable attribute otherwise it doesn't go through mangling? Then I guess you only need a check for Linux or Windows cases.

26

Oh I see. I think it would be good to explain this better.

erichkeane marked 3 inline comments as done.Mar 25 2020, 12:26 PM

Both done here: fe5c719eaf572e23b700e75832ec37a3761b337b . Thanks for the review @Anastasia

clang/test/CodeGenOpenCL/pipe_types_mangling.cl
13

Done, I wasn't able to remove OCLWINDOWS, since windows mangles attribute 'overloadable' in C only (and thus OpenCL).

Anastasia added inline comments.Mar 26 2020, 5:56 AM
clang/test/CodeGenOpenCL/pipe_types_mangling.cl
13

Sorry what I meant is we should probably add overloadable attr to all functions so that we can check mangling in OpenCL C too. Then we won't need to check UNMANGLED.

I am very confused of why right now you only do it in some cases but not all...

20

Do you mean overload on pipe element type?

Anastasia added inline comments.Mar 26 2020, 6:08 AM
clang/test/CodeGenOpenCL/pipe_types_mangling.cl
13

I just noticed that my earlier comment has been addressed in https://reviews.llvm.org/rGfe5c719eaf57 but it didn't show on this review. So we are fine now! Thanks for looking at this.