[PATCH] D125555: [clang] Add __has_target_feature

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon May 16 05:35:03 PDT 2022


aaron.ballman added a comment.

It's not clear to me why existing facilities shouldn't be extended to cover this case rather than coming up with another feature testing macro. There's already plenty of confusion for users to decide between `__has_feature` and `__has_extension`, and now we're talking about adding a third choice to the mix.



================
Comment at: clang/docs/LanguageExtensions.rst:260
 
+``__has_target_feature``
+------------------------
----------------
The first question that comes to mind for me is: why is `__has_feature` not sufficient?


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


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

https://reviews.llvm.org/D125555



More information about the cfe-commits mailing list