[PATCH] D116549: [OpenMP][Clang] Allow passing target features in ISA trait for metadirective clause

[Saiyedul Islam via Phabricator via cfe-commits]

saiislam 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),
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116549