jdoerfert added inline comments.
================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:2533
+ std::function<void(StringRef)> DiagUnknownTrait = [this, Loc](
+ StringRef ISATrait) {};
+ TargetOMPContext OMPCtx(ASTContext, std::move(DiagUnknownTrait),
----------------
saiislam wrote:
> jdoerfert wrote:
> > jdoerfert wrote:
> > > saiislam wrote:
> > > > jdoerfert wrote:
> > > > > Why doesn't this diagnose nothing?
> > > > Because an isa-feature will fail at least once, for either host
> > > > compilation or device compilation. So, no point in always giving a
> > > > warning.
> > > That is debatable.
> > >
> > > First, if I compile for a single architecture there is no device
> > > compilation and it should warn.
> > > Second, if I place the metadirective into a declare variant function or
> > > add a `kind(...)` selector to it it will also not warn even if you have
> > > multiple architectures.
> > >
> > >
> > ```
> > ASTContext &Context = getASTContext();
> > std::function<void(StringRef)> DiagUnknownTrait = [this,
> > ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ CE](StringRef
> > ISATrait) {
> > ┊ // TODO Track the selector locations in a way that is accessible here
> > to
> > ┊ // improve the diagnostic location.
> > ┊ Diag(CE->getBeginLoc(), diag::warn_unknown_declare_variant_isa_trait)
> > ┊ ┊ ┊ << ISATrait;
> > };
> > TargetOMPContext OMPCtx(Context, std::move(DiagUnknownTrait),
> >
> >
> >
> > ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ getCurFunctionDecl(),
> > DSAStack->getConstructTraits());
> > ```
> > Already exists (SemaOpenMP). Why do we need a second, different diagnostic?
> Isn't giving a remark better than a warning, when we know in many cases this
> will be hit during a normal (expected) compilation for target offload?
> Remark diagnostic will give ample handle for understanding the flow without
> the need to explicitly deal with this warning during compilation of user
> programs.
>
> I am fine changing it to a warning if you feel strongly about this.
1) Having two diagnostics for the same thing is generally bad and there doesn't
seem to be a reason why we would need/want to treat metadirective and declare
variant differently.
Thus, if we want to move to remarks we should move the second diagnostic as
well. Cleaning up existing code is more important than adding new code.
2) I still don't see why this is "normal" or "expected" at all. The warning
reads:
`isa trait 'foo' is not known to the current target; verify the spelling or
consider restricting the context selector with the 'arch' selector further`.
Hence, `device={isa("flat..."), arch(amdgcn)}` should not cause a warning even
if you compile it for the host, an nvidia gpu, or anything other than AMDGCN.
FWIW, this is [supposed to be] ensured by the positioning of the isa trait
selector in OMPKinds.def:
```
// Note that we put isa last so that the other conditions are checked first.
// This allows us to issue warnings wrt. isa only if we match otherwise.
__OMP_TRAIT_SELECTOR(device, isa, true)
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116549/new/
https://reviews.llvm.org/D116549
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
