Page MenuHomePhabricator

[OpenCL] Add support of __opencl_c_generic_address_space feature macro
ClosedPublic

Authored by azabaznov on May 31 2021, 4:31 AM.

Diff Detail

Event Timeline

azabaznov created this revision.May 31 2021, 4:31 AM
azabaznov requested review of this revision.May 31 2021, 4:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2021, 4:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
svenvh added inline comments.Jun 1 2021, 2:25 AM
clang/lib/Basic/TargetInfo.cpp
405

This means we now have two separate places that set OpenCLGenericAddressSpace, the other place being in CompilerInvocation::setLangDefaults(). That feels like a maintenance hazard. Do you think it makes sense to set this field in one single place instead?

Anastasia added inline comments.Jun 1 2021, 4:55 AM
clang/lib/Basic/TargetInfo.cpp
405

I think we should try to set it in CompilerInvocation.cpp directly, we should be able to query TargetOptions there. Although that place is expected to be for the language-specific defaults but we broke the standard flow by having the language mode controlled by the target settings anyway.

I can't remember though why we have decided to add dedicated LangOpts entries for generic address space instead of just using OpenCLOptions from Sema? I think it simplifies the handling of some builtin functions?

azabaznov added inline comments.Jun 1 2021, 5:16 AM
clang/lib/Basic/TargetInfo.cpp
405

This means we now have two separate places that set OpenCLGenericAddressSpace, the other place being in CompilerInvocation::setLangDefaults(). That feels like a maintenance hazard. Do you think it makes sense to set this field in one single place instead?

I think we should try to set it in CompilerInvocation.cpp directly, we should be able to query TargetOptions there.

I don't think that we are able to access target options at that stage without modifying current interfaces. CompilerInvocation::setLangDefaults() is a static member function.

I can't remember though why we have decided to add dedicated LangOpts entries for generic address space instead of just using OpenCLOptions from Sema? I think it simplifies the handling of some builtin functions?

That's correct. Also, the idea was to reuse generic keyword in other languages.

Anastasia added inline comments.Jun 1 2021, 8:25 AM
clang/lib/Basic/TargetInfo.cpp
405

I don't think that we are able to access target options at that stage without modifying current interfaces. CompilerInvocation::setLangDefaults() is a static member function.

I wonder if the function is static due to an old interface or something because it seems to be only called from CompilerInvocation::ParseLangArgs which isn't a static member as far as I can see. I wonder if it should just become a non-static member instead?

azabaznov added inline comments.Jun 1 2021, 11:10 AM
clang/lib/Basic/TargetInfo.cpp
405

Actually CompilerInvocation::ParseLangArgs is static as well. Anyway, I think this change would be really impactful. I believe ::adjust is a prefect place to coordinate language options with target information (at least for now).

Anastasia added inline comments.Jun 9 2021, 4:55 AM
clang/lib/Basic/TargetInfo.cpp
405

Apart from that ::adjust was just introduced as a hack to mitigate the fact that the original clang flow was designed to set the types differently and now we are extending the hack to cover more areas.

Technically to adhere to clang flow we should probably set those options as target options but when we prototyped that it was too hard to condition the clang builtins on such options or something?

This way or another there seems to be more refactoring needed to preserve the flow fully. I think this at least deserves a FIXME comment to explain why we are ending up with the design that requires overriding the LangOpts here and also highlighting the fact that we have two places where it is set.

azabaznov updated this revision to Diff 355873.Thu, Jul 1, 7:24 AM

Added FIXME. Alternatively, we could add this language option only in ::adjust if there is still a worry about multiple places of definitions. Btw, there will some more language options which dependent on target settings for OpenCL C 3.0, such as pipes and blocks

azabaznov marked an inline comment as done.Thu, Jul 1, 7:24 AM
Anastasia accepted this revision.Mon, Jul 12, 4:22 AM

LGTM! Thanks

This revision is now accepted and ready to land.Mon, Jul 12, 4:22 AM