[PATCH] D97058: [OpenCL] Refactor diagnostic for OpenCL extension/feature

Wed Mar 10 05:34:30 PST 2021

Anastasia added a comment.

In D97058#2615926 <https://reviews.llvm.org/D97058#2615926>, @azabaznov wrote:

> Corrected some mistakes, added a test for diagnosing undeclared identifiers when an extension is unsupported. Generally leaving the change as it is as completely removing pragma may break backward compatibility now: let's do it in a separate patch and notify everyone before doing that.

Ok, addressing in a separate patch is reasonable, but why do you think that we will break backward compatibility? My current worry is that the implementation is so messy and inconsistent that it will take us longer time if  we do the incremental steps. Also, we risk introducing the regressions since it is so hard to make sense out of it.

> Also, we should wait before spec will be finalized.

This issue has been reported 2 years ago but there was very little progress made since then. So I assume it is not important and I am not convinced it would be finalized any time soon. In either case, it would be reasonable for us to simplify the implementation. Generally, we don't guarantee backward compatibility to non-conformant functionality but however, in this case I don't see what would be broken if we remove the redundant diagnostics that were never intended and only polluted the source code without providing any benefit to the developers. The existing kernels would still compile?

> Note, that adding a test for diagnosing extended atomic types is hard because parser diagnoses a typo (since some types, such as atomic_int, are already present), so diagnostic messages look awkward.

================
Comment at: clang/lib/Sema/Sema.cpp:339
+          setOpenCLExtensionForType(AtomicDoubleT, "cl_khr_fp64");
+          Atomic64BitTypes.push_back(AtomicDoubleT);
+        }
----------------
Side comment: I don't see why would `atomic_double` have anything to do with `cl_khr_int64_base_atomics` or `cl_khr_int64_extended_atomics`? If anything the extensions should have been named differently...

================
Comment at: clang/lib/Sema/Sema.cpp:376
+    addImplicitTypedef(#ExtType, Context.Id##Ty);                              \
+    setOpenCLExtensionForType(Context.Id##Ty, #Ext);                           \
+  }
----------------
Anastasia wrote:
> Since we are guarding by the feature/extension support we could even remove the need for pragma in order to use the types?
Ok, image types are reserved so we still need the diagnostic although at some point we should just implement them as all other reserved identifiers i.e. using keywords. Then we would not need any special handling in OpenCL at all.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97058/new/

https://reviews.llvm.org/D97058