Page MenuHomePhabricator

[OpenCL] Refactor diagnostic for OpenCL extension/feature
ClosedPublic

Authored by azabaznov on Feb 19 2021, 8:51 AM.

Details

Summary

There is no need to check for enabled pragma for core or optional core features,
thus this check is removed

Diff Detail

Event Timeline

azabaznov created this revision.Feb 19 2021, 8:51 AM
azabaznov requested review of this revision.Feb 19 2021, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2021, 8:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

FYI, I would even be ok if we remove the need for enabling non-core features too but it is also fine to start from just core features. Thanks!

clang/lib/Sema/Sema.cpp
360

I think the same should apply to double not only atomic_double?

FYI, I would even be ok if we remove the need for enabling non-core

AFAIU this can be done not for every extension to maintain backward compatibility. In this case I think https://reviews.llvm.org/D97052 can be useful.

Also, I imagine that implicit type definition is not  needed if no pragma required.

clang/lib/Sema/Sema.cpp
360

I think double is created in a place where it's not possible to guard it with OpenCL extensions support. I will double-check.

FYI, I would even be ok if we remove the need for enabling non-core

AFAIU this can be done not for every extension to maintain backward compatibility.

This should not result in any backward compatibility issues, as the old kernels using pragma enable will still compile successfully.

Also, I imagine that implicit type definition is not needed if no pragma required.

They need to be added but conditionally on language version or extension/feature support. If there is no support they should not be added as identifiers by the compiler and therefore can be reused by the developers in the application code.

clang/lib/Sema/Sema.cpp
360

That's right, double is a reserved identifier so we should always accept it. However, we should allow its use as a legal type without the pragma if fp64 is supported. We could remove this statement here and add a separate check for it in Sema, but we could also modify this mechanism to check for the extensions/features being supported instead of being enabled.

azabaznov updated this revision to Diff 327767.Mar 3 2021, 6:27 AM

Check 'isEnabled' is now private: it is used only for non-core or non-optional core features;
creation of implicit type definitions is guarder with extension support check; minor refactoring

azabaznov added inline comments.Mar 3 2021, 6:38 AM
clang/lib/Basic/OpenCLOptions.cpp
22–26

Note that I can't use isWithPragma from https://reviews.llvm.org/D97052 as some extensions are promoted to core or optional core in different versions

Check 'isEnabled' is now private: it is used only for non-core or non-optional core features;
creation of implicit type definitions is guarder with extension support check; minor refactoring

Ok, I think we could remove further the differences between extensions and features wrt pragmas as suggested in the comments but it is also ok if we do separately. This is definitely a step closer to the end goal of simplifying and unifying the handling of extensions and features and removing the confusion about the pragmas.

clang/lib/Sema/Sema.cpp
365

I think this should be lifted into the above if statement? Although since we are adding the types conditionally we could even drop this code completely and let the developers just use the types as they are already added and made available by the frontend.

377

Since we are guarding by the feature/extension support we could even remove the need for pragma in order to use the types?

azabaznov added inline comments.Mar 4 2021, 1:03 AM
clang/lib/Sema/Sema.cpp
365

I think this should be lifted into the above if statement?

No, this is needed to be here as atomic_double added above.

Although since we are adding the types conditionally we could even drop this code completely and let the developers just use the types as they are already added and made available by the frontend.

I'm now worried about diagnostics with that change:

tmp.cl:2:17: error: expected ';' after expression
    atomic_ulong a;
                ^
                ;
tmp.cl:2:5: error: use of undeclared identifier 'atomic_ulong'
    atomic_ulong a;
    ^
tmp.cl:2:18: error: use of undeclared identifier 'a'
    atomic_ulong a;
                 ^
tmp.cl:3:5: error: unknown type name 'atomic_uintptr_t'; did you mean 'atomic_uint'?
    atomic_uintptr_t u;
    ^~~~~~~~~~~~~~~~
    atomic_uint
note: 'atomic_uint' declared here
4 errors generated.

Is this expected behaviour when cl_khr_int64_base_atomics and 'cl_khr_int64_extended_atomics' not supported? I think we need o be more careful with reserved identifiers. However, I didn't find explicit wording about it, but I'm pretty sure that all of these atomic types identifiers should be reserved.

Anastasia added inline comments.Mar 5 2021, 5:03 AM
clang/lib/Sema/Sema.cpp
365

Well, the reserved types are those listed in 6.3.4. Reserved Data Types.

These are typical examples of library types that are added in the standard headers outside of the compiler even though it was done in the clang source code in this case. Theoretically, they should have been added via an include file but in OpenCL it was supposed to be replaced by the pragma which has never happened for various reasons.

Anyway I think we should just either add the types and let them be used w/o any pragma magic or not add those and allow the identifiers to be reused in the application kernels. This is how atomic builtin functions behave now (e.g. atomic_init). The types should not be different.

Corrected some mistakes, added a test for diagnosing undeclared identifiers when a 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. Also, we should wait before spec will be finalized.

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.

Replaced atomic_double implicit definition

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.

clang/lib/Sema/Sema.cpp
339

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...

377

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.

Anastasia accepted this revision.Mar 10 2021, 5:36 AM

LGTM! Although some further improvements seem to be necessary they can be done separately.

This revision is now accepted and ready to land.Mar 10 2021, 5:36 AM

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.

As regarding backward compatibility -- they may be kernels which already rely on pragmas. So maybe their users already expect to see something like: some_type requires some_ext to be enabled. If we remove check for pragma their will be no diagnostic at all

This issue has been reported 2 years ago but there was very little progress made since then.

I see :( Maybe we should introduce diagnostic, something like: pragma enable is deprecated: there is no need to enable extension to use optional functionally. But anyway it seems really impactful as current specification allows using pragma...

clang/lib/Sema/Sema.cpp
339

Yeah... With the fact given that atomic_int for example by default. Intuitively I expected to see only cl_khr_fp64 required.

OpenCL C 3.0 spec, 6.15.12.6. Atomic integer and floating-point types:
...
atomic_double [54]
...
[54]: The atomic_double type is only supported if double precision is supported and the cl_khr_int64_base_atomics and cl_khr_int64_extended_atomics extensions are supported and have been enabled. If this is the case then an OpenCL C 3.0 compiler must also define the __opencl_c_fp64 feature.

Does it seem like spec issue/inaccuracy?

This revision was landed with ongoing or failed builds.Mar 12 2021, 12:44 AM
This revision was automatically updated to reflect the committed changes.

@azabaznov This has broken some buildbots http://lab.llvm.org:8011/#/builders/105/builds/6855 - can you take a look please?

Hi,

this breaks check-clang on arm macs: http://45.33.8.238/macm1/5421/step_6.txt

(The fatal error: 'opencl-c-base.h' file not found bit is printed when the test passes too, and is unrelated.)

Hi!

Yeah,  I'm looking into it, but I can't reproduce it locally: the test passes at x86_64 linux system. I'll revert the change if it takes too much time to investigate what's going on. Thanks.

Does it repro if you add -target arm64-apple-macosx as arg to c-index-test on the RUN line of that test?

Here's the output of that RUN line on an arm mac when the test fails: http://pastie.org/p/1go1Y9WXjs5T371RtyJ3Gi

Let's revert for now, things have been broken for over 9 hours already.

Does it repro if you add -target arm64-apple-macosx as arg to c-index-test on the RUN line of that test?

I also had trouble reproducing the failure locally, but by specifying this target I can reproduce the failure.

That means the fix is simple then? Simply add -target spir to the test.

It repros for me with this local diff

% git diff
diff --git a/clang/test/Index/opencl-types.cl b/clang/test/Index/opencl-types.cl
index 496f38752fa2..13f6058864b5 100644
--- a/clang/test/Index/opencl-types.cl
+++ b/clang/test/Index/opencl-types.cl
@@ -1,4 +1,4 @@
-// RUN: c-index-test -test-print-type %s -cl-std=CL2.0 | FileCheck %s
+// RUN: c-index-test -test-print-type %s -cl-std=CL2.0 -target arm64-apple-macosx | FileCheck %s

 #pragma OPENCL EXTENSION cl_khr_fp16 : enable
 #pragma OPENCL EXTENSION cl_khr_fp64 : enable

The failure goes away if I locally revert the change.

I don't know if it's expected that the test added in https://reviews.llvm.org/D51484 starts failing after this change for arm triples. If it is, then just adding a fixed triple is fine. If it's unexpected, the let's revert and analyze async, with a green tree.

Does it repro if you add -target arm64-apple-macosx as arg to c-index-test on the RUN line of that test?

I also had trouble reproducing the failure locally, but by specifying this target I can reproduce the failure.

That means the fix is simple then? Simply add -target spir to the test.

I can't get how this test could even work for other targets since it is using Intel's extension successfully. My guess is that -target spir might indeed be a proper fix. However, I am confused why it only started failing on this commit.

Thanks guys,

I'm on way to trying that, just building clang from scratch.

  If it is, then just adding a fixed triple is fine.

Yeah, it is expected as this change removes types which require extension from parsers state. X86 and SPIR support all extensions by default, so that's why mce_payload is there. I'll provide the fix to the test once I'll try that locally.

(The fatal error: 'opencl-c-base.h' file not found bit is printed when the test passes too, and is unrelated.)

I can't reproduce this locally neither with nor without this commit.

I think there is something wrong with the way this test runs, this might explain why it didn't show up as failing even though it could not have been parsed successfully with Arm or Amd triples.

In case it's useful, here's the output of the RUN line on an m1 mac with this patch locally reverted (where the test passes): http://pastie.org/p/2F3Y0xUMtH5RP9TVRzG4LI

In case it's useful, here's the output of the RUN line on an m1 mac with this patch locally reverted (where the test passes): http://pastie.org/p/2F3Y0xUMtH5RP9TVRzG4LI

Thanks, this seems strange as we are not supposed to have Intel specific extensions exposed for Arm:

VarDecl=mce_payload:131:37 (Definition) (invalid) [type=__private intel_sub_group_avc_mce_payload_t] [typekind=Typedef] [canonicaltype=__private intel_sub_group_avc_mce_payload_t] [canonicaltypekind=OCLIntelSubgroupAVCMcePayload] [isPOD=1] [isAnonRecDecl=0]
TypeRef=intel_sub_group_avc_mce_payload_t:0:0 [type=intel_sub_group_avc_mce_payload_t] [typekind=Typedef] [canonicaltype=intel_sub_group_avc_mce_payload_t] [canonicaltypekind=OCLIntelSubgroupAVCMcePayload] [isPOD=1] [isAnonRecDecl=0]

It feels like we aren't doing something right either in default headers or in the frontend setup.

In case it's useful, here's the output of the RUN line on an m1 mac with this patch locally reverted (where the test passes): http://pastie.org/p/2F3Y0xUMtH5RP9TVRzG4LI

Thanks, this seems strange as we are not supposed to have Intel specific extensions exposed for Arm:

VarDecl=mce_payload:131:37 (Definition) (invalid) [type=__private intel_sub_group_avc_mce_payload_t] [typekind=Typedef] [canonicaltype=__private intel_sub_group_avc_mce_payload_t] [canonicaltypekind=OCLIntelSubgroupAVCMcePayload] [isPOD=1] [isAnonRecDecl=0]
TypeRef=intel_sub_group_avc_mce_payload_t:0:0 [type=intel_sub_group_avc_mce_payload_t] [typekind=Typedef] [canonicaltype=intel_sub_group_avc_mce_payload_t] [canonicaltypekind=OCLIntelSubgroupAVCMcePayload] [isPOD=1] [isAnonRecDecl=0]

It feels like we aren't doing something right either in default headers or in the frontend setup.

Oh, I just realized that I already have a fix for this https://reviews.llvm.org/D92244. I better commit this on Monday though. :)

I can't find why and how opencl-c-base.h is used for the test. But it feels it is adding the random noise to the testing as it is not expected to be used. I don't feel that this commit is affecting the header though, so it is something that might have already been broken and hasn't been noticed.

Yes, I was able to reproduce the failure with -target arm64-apple-macosx flag, I provided the fix to set explicit target: https://reviews.llvm.org/D98539.

@Anastasia, I tried to cherry-pick https://reviews.llvm.org/D92244, but the error is still reproducible.

Yes, I was able to reproduce the failure with -target arm64-apple-macosx flag, I provided the fix to set explicit target: https://reviews.llvm.org/D98539.

I have approved it. Thanks!

@Anastasia, I tried to cherry-pick https://reviews.llvm.org/D92244, but the error is still reproducible.

Do you also get fatal error: 'opencl-c-base.h' file not found? If so, we might need at least to file a clang bug that we should look at before the next release.

I'm going to revert this in 30 minutes. The tree has been red for 12h due to this then.

I'm going to revert this in 30 minutes. The tree has been red for 12h due to this then.

I have committed the fix mentioned above. Apology for the delay.

Do you also get fatal error: 'opencl-c-base.h' file not found? If so, we might need at least to file a clang bug that we should look at before the next release.

I'm able to reproduce it when not setting -target option explicitly:

$ ./build_release/bin/c-index-test -test-print-type ./llvm-project/clang/test/Index/opencl-types.cl -cl-std=CL2.0 | ./build_release/bin/FileCheck ./llvm-project/clang/test/Index/opencl-types.cl
fatal error: 'opencl-c-base.h' file not found
fatal error: 'opencl-c-base.h' file not found

$ ./build_release/bin/c-index-test -test-print-type ./llvm-project/clang/test/Index/opencl-types.cl -cl-std=CL2.0 -target x86_64 | ./build_release/bin/FileCheck ./llvm-project/clang/test/Index/opencl-types.cl
$