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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
