[PATCH] D42983: [clang-tidy] Add readability-simd-intrinsics check.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked 2 inline comments as done.
MaskRay added inline comments.


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:75
+  // libcxx implementation of std::experimental::simd requires at least C++11.
+  if (!Result.Context->getLangOpts().CPlusPlus11)
+    return;
----------------
lebedev.ri wrote:
> Is it reasonable to suggest to use `<experimental/*>`?
> I would guess it should be `CPlusPlus2a`
Added a check-specific option `readability-simd-intrinsics.Experiment`.




================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:84
+  // SIMD intrinsic call.
+  const FunctionDecl *Callee = Call->getDirectCallee();
+  if (!Callee)
----------------
lebedev.ri wrote:
> I would refactor this as astmatcher, at least a local one, e.g. something like
> ```
> AST_MATCHER(CallExpr, hasDirectCallee) {
>   return Node.getDirectCallee();
> }
> ```
> +
> ```
> void SIMDIntrinsicsCheck::registerMatchers(MatchFinder *Finder) {
>   Finder->addMatcher(
>       callExpr(callee(
> +            allOf(
>                      functionDecl(matchesName("^::(_mm_|_mm256_|_mm512_|vec_)")
> +                  , hasDirectCallee()
> +            )
>                )),
>                unless(isExpansionInSystemHeader()))
>           .bind("call"),
>       this);
> }
> ```
> Unless of course there is already a narrower that does that
```
AST_MATCHER(CallExpr, hasDirectCallee) {
  return Node.getDirectCallee();
}
```

looks too overkill and I still have to call `Call->getDirectCallee()` in `SIMDIntrinsicsCheck::check` and then `Callee->getName()`. I decide to keep it as is.


That said, I should also study why `AST_MATCHER(CallExpr, hasDirectCallee)` does not compile:
```
../tools/clang/tools/extra/clang-tidy/readability/SIMDIntrinsicsCheck.cpp:95:16: error: call to 'callee' is ambiguous
      callExpr(callee(allOf(functionDecl(allOf(
               ^~~~~~
../tools/clang/include/clang/ASTMatchers/ASTMatchers.h:2811:25: note: candidate function
AST_MATCHER_P(CallExpr, callee, internal::Matcher<Stmt>,
                        ^
../tools/clang/include/clang/ASTMatchers/ASTMatchers.h:2827:34: note: candidate function
AST_MATCHER_P_OVERLOAD(CallExpr, callee, internal::Matcher<Decl>, InnerMatcher,
```


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:87
+    return;
+  bool IsVector = Callee->getReturnType()->isVectorType();
+  for (const ParmVarDecl *Parm : Callee->parameters()) {
----------------
lebedev.ri wrote:
> Same here, i *think* something like this would be better?
> ```
> AST_MATCHER(FunctionDecl, isVectorFunction) {
>  bool IsVector = Node.getReturnType()->isVectorType();
>   for (const ParmVarDecl *Parm : Node.parameters()) {
>     QualType Type = Parm->getType();
>     if (Type->isPointerType())
>       Type = Type->getPointeeType();
>     if (Type->isVectorType())
>       IsVector = true;
> 
>   return IsVector;
> }
> ```
> +
> ```
> void SIMDIntrinsicsCheck::registerMatchers(MatchFinder *Finder) {
>   Finder->addMatcher(
>       callExpr(callee(
>             allOf(
>                      functionDecl(
> +                   allOf(
>                          matchesName("^::(_mm_|_mm256_|_mm512_|vec_)")
> +                     , isVectorFunction()
> +                   )
>                   , hasDirectCallee()
>             )
>                )),
>                unless(isExpansionInSystemHeader()))
>           .bind("call"),
>       this);
> }
> ```
Thanks! Added isVectorFunction. I guess I may should use unnamed namespace for AST_MATCHER, but do I also need the unnamed namespace to enclose TrySuggestPPC and TrySuggestX86?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42983