Page MenuHomePhabricator

[OpenCL] Pretty print __private addr space
ClosedPublic

Authored by Anastasia on Dec 10 2019, 7:33 AM.

Details

Summary

Historically Clang didn't have representation of __private and therefore it has never been printed in types. It appeared to be the same as no address space and appeared to be confusing to the users. Also it was unnecessarily exposing the implementation details of Clang.

We have added a representation of it in AST https://reviews.llvm.org/D35082 but it hasn't been added to the type printing yet.

Current patch adds printing of __private in type printer. The main motivation for this are:

  1. Avoid situations where __private addr space is missing in the type of diagnostics even if it was provided in the source code.
  2. Improve frontend debugging support because we will now be able to differentiate between __private and absence of addr space in AST dumps.

There is one potential issue that some diagnostics will become more verbose with extra __private addr spaces being printed that were deduced implicitly. This aligns however with the rest of behaviour i.e. deduced __generic addr space is currently printed in AST dump and diagnostics. It is also reasonable for deduced addr spaces to appear in type printing because it can be explained by the language rules. However, absence of __private from type printing has been an implementation detail of Clang that never was intuitive to the users.

Diff Detail

Event Timeline

Anastasia created this revision.Dec 10 2019, 7:33 AM

LGTM, but I'd like someone who works on OpenCL front-end to approve.
+@AlexeySotkin

svenvh accepted this revision.Dec 12 2019, 10:25 AM

LGTM.

This revision is now accepted and ready to land.Dec 12 2019, 10:25 AM
AlexeySotkin added inline comments.Dec 13 2019, 12:15 AM
clang/test/SemaOpenCL/access-qualifier.cl
28

Minor. An error message like this looks a bit confusing to me. User might wonder whether parameters are incompatible because of address space or because of access qualifiers. Should it print passing '__private img1d_wo' (aka '__private __write_only image1d_t') to parameter of incompatible type '__private __read_only image1d_t'? In this case it is clear that there is a mismatch in access qualifiers.

clang/test/SemaOpenCL/address-spaces-conversions-cl2.0.cl
373

Again, I think it would be more clear if it was: passing '__constant int *__private' to parameter of type '__generic int *__private' changes address space of pointer

Anastasia marked an inline comment as done.Dec 13 2019, 5:15 AM
Anastasia added inline comments.
clang/test/SemaOpenCL/access-qualifier.cl
28

Yes, I agree. However, we are printing the full QualType in both places in this diagnostics, so the problem is that the addr space is either not deduced or being dropped from QualType somewhere. I don't think we have got addr spaces working yet for all cases correctly and printing __private have revealed a number of such issues. I suggest however to fix them in isolation case by case as we discover them. This commit is already pretty big and I don't want to expand it even more. I have opened a bug to track this issues: https://bugs.llvm.org/show_bug.cgi?id=44294
Hopefully we can fix it asap. Does it make sense?

AlexeySotkin added inline comments.Dec 13 2019, 5:34 AM
clang/test/SemaOpenCL/access-qualifier.cl
28

Yes, fair enough.

AlexeySotkin accepted this revision.Dec 13 2019, 5:34 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptDec 27 2019, 5:44 AM