yaxunl marked 2 inline comments as done.
yaxunl added inline comments.
================
Comment at: clang/docs/LanguageExtensions.rst:260
+``__has_target_feature``
+------------------------
----------------
aaron.ballman wrote:
> The first question that comes to mind for me is: why is `__has_feature` not
> sufficient?
`__has_target_feature` accepts predefined target features for the specified
target triple and CPU. The names of target features are determined by the
target. They may overlap with clang features and cause ambiguity. Another issue
is that they usually contain '-', which makes them not valid identifiers of
C/C++, therefore the builtin macro has to accept a string literal argument
instead of identifier argument like `__has_feature`.
================
Comment at: clang/docs/LanguageExtensions.rst:275
+ // On amdgcn target
+ #if __has_target_feature("s-memtime-inst")
+ x = __builtin_amdgcn_s_memtime();
----------------
aaron.ballman wrote:
> yaxunl wrote:
> > tra wrote:
> > > Do you have a better example? This particular case could've been handled
> > > by existing `__has_builtin()`.
> > >
> > > While I could see usefulness of checking features (e.g. for CUDA/NVPTX it
> > > could be used to chose inline assembly supported only by newer PTX
> > > versions, but even then that choice could be made using existing methods,
> > > even if they are not always direct (e.g. by using CUDA_VERSION as a proxy
> > > for "new enough PTX version").
> > >
> > `__has_builtin` returns 1 as long as the builtin is known to clang, even if
> > the builtin is not supported by the target CPU. This is because the
> > required target feature for a builtin is in ASTContext, whereas
> > `__has_builtin` is evaluated in preprocessor, where the information is not
> > known.
> > `__has_builtin` returns 1 as long as the builtin is known to clang, even if
> > the builtin is not supported by the target CPU. This is because the
> > required target feature for a builtin is in ASTContext, whereas
> > `__has_builtin` is evaluated in preprocessor, where the information is not
> > known.
>
> Why is that not a deficiency with `__has_builtin` that we should fix?
It is arguable whether this is a bug for `__has_builtin`. I tend to treat it as
a bug and think it should be fixed. However, fixing it may cause regressions
since there may be existing code expecting `__has_builtin` not depending on
availability of required target feature. This will limit the usability of a fix
for this issue.
Even if we agree that this is a bug for `__has_builtin` which should be fixed,
this does conflict with adding `__has_target_feature`, as
`__has_target_feature` has more uses than determining whether a target builtin
is available, e.g. choosing different algorithms based on availability of
certain target feature, or using certain inline assembly code.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125555/new/
https://reviews.llvm.org/D125555
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
