[PATCH] D125555: [clang] Add __has_target_feature

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.


================
Comment at: clang/docs/LanguageExtensions.rst:275
+  // On amdgcn target
+  #if __has_target_feature("s-memtime-inst")
+    x = __builtin_amdgcn_s_memtime();
----------------
yaxunl wrote:
> 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.
> 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.

sorry typo. this does not conflict


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

https://reviews.llvm.org/D125555