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

[Johannes Doerfert via Phabricator via cfe-commits]

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