rjmccall added inline comments.
================
Comment at: clang/include/clang/Sema/Sema.h:12248
+ /// SYCLDiagIfDeviceCode(Loc, diag::err_type_unsupported) << "__float128";
+ DeviceDiagBuilder SYCLDiagIfDeviceCode(SourceLocation Loc, unsigned DiagID);
+
----------------
Fznamznon wrote:
> rjmccall wrote:
> > Will this collect notes associated with the diagnostic correctly?
> Could you please make your question a bit more concrete?
> This function is supposed to work in the same way as
> `Sema::CUDADiagIfDeviceCode` and `Sema::diagIfOpenMPDeviceCode` . It emits
> given diagnostic if the current context is known as "device code" and makes
> this diagnostic deferred otherwise. It uses the `DeviceDiagBuilder` which was
> implemented earlier. This `DeviceDiagBuilder` also tries to emit callstack
> notes for the given diagnostics. Do you mean these callstack notes or
> something else?
Logically, notes that are emitted after a warning or error are considered to be
part of that diagnostic. A custom `DiagBuilder` that only redirects the main
diagnostic but allows the notes to still be emitted will effectively cause
those notes to misleadingly follow whatever previous diagnostic might have been
emitted.
I call this out specifically because some of the places where you're using this
still seem to try to emit notes afterwards, at least in some cases. It's
possible that `CUDADiagIfDeviceCode` happens to not be used in such a way.
Really I'm not sure this conditional `DiagBuilder` approach was a good idea the
first time, and I think we should probably reconsider rather than duplicating
it.
================
Comment at: clang/lib/Sema/SemaAvailability.cpp:479
+ case UnavailableAttr::IR_SYCLForbiddenType:
+ diag_available_here = diag::err_type_unsupported;
+ break;
----------------
Fznamznon wrote:
> rjmccall wrote:
> > All of the other cases are setting this to a note, not an error, so I
> > suspect this will read wrong.
> Yes, this is not a note. For such samples:
>
> ```
> int main() {
> __float128 CapturedToDevice = 1;
> kernel<class variables>([=]() {
> decltype(CapturedToDevice) D;
> });
> }
> ```
> It looks like this:
> ```
> float128.cpp:63:14: error: 'CapturedToDevice' is unavailable
> decltype(CapturedToDevice) D;
> ^
> float128.cpp:59:14: error: '__float128' is not supported on this target ///
> This emitted instead of note
> __float128 CapturedToDevice = 1;
> ^
> ```
> I had feeling that it should probably be a note. But there is no implemented
> note for unsupported types. I think I can add a new one if it will make it
> better. Should I?
Yeah, this should be a note, like "note: variable is unavailable because it
uses a type '__float128' that is not supported on this target". You should add
that.
================
Comment at: clang/lib/Sema/SemaAvailability.cpp:534
+ if (S.getLangOpts().SYCLIsDevice)
+ S.SYCLDiagIfDeviceCode(Loc, diag) << ReferringDecl;
+ else
----------------
Fznamznon wrote:
> rjmccall wrote:
> > Are you sure you want to be applying this to all of the possible
> > diagnostics here, rather than just for SYCLForbiddenType unavailable
> > attributes?
> I suppose it is reasonable if we want to reuse unavaliable attribute for
> other SYCL use cases. Plus, In SYCL we don't know where is device code until
> we instantiate templates, it happens late, so we have to defer any diagnostic
> while compiling for device, otherwise we can point to host code where much
> more is allowed.
My point is actually the reverse of that. This code path is also used for
normal `unavailable` attributes, not just the special ones you're synthesizing.
Diagnostics from the use of explicitly-unavailable declarations shouldn't get
any special treatment here, no more than you'd give special treatment to a
diagnostic arising from an attempt to assign a pointer into a `float`. In the
logic above where you recognize `IR_SYCLForbiddenType`, I think you should just
check whether you should transitively defer the diagnostic and, if so, do so
and then bail out of this function early. That might mean you don't need the
custom DiagBuilder, too.
================
Comment at: clang/lib/Sema/SemaDecl.cpp:18030
+ if (LangOpts.SYCLIsDevice && FD->hasAttr<SYCLKernelAttr>())
+ return FunctionEmissionStatus::Emitted;
+
----------------
Fznamznon wrote:
> rjmccall wrote:
> > So you want to emit it for the definition in addition to emitting it for
> > specific specializations?
> Somehow diagnostics are emitted only for the definitions.
> Without this change diagnostics aren't emitted at all.
Hmm. We might be marking the template pattern invalid; that could result in
all sorts of diagnostics being suppressed. We definitely shouldn't be marking
things invalid without emitting an eager diagnostic.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D74387/new/
https://reviews.llvm.org/D74387
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
