This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add readability-simd-intrinsics check.
ClosedPublic

Authored by MaskRay on Feb 6 2018, 1:46 PM.

Details

Summary

Many architectures provide SIMD operations (e.g. x86 SSE/AVX, Power AltiVec/VSX,
ARM NEON). It is common that SIMD code implementing the same algorithm, is
written in multiple target-dispatching pieces to optimize for different
architectures or micro-architectures.

The C++ standard proposal P0214 and its extensions cover many common SIMD
operations. By migrating from target-dependent intrinsics to P0214 operations,
the SIMD code can be simplified and pieces for different targets can be unified.

Refer to http://wg21.link/p0214 for introduction and motivation for the
data-parallel standard library.

Event Timeline

MaskRay created this revision.Feb 6 2018, 1:46 PM
MaskRay added a subscriber: timshen.
MaskRay updated this revision to Diff 133083.Feb 6 2018, 2:29 PM

Add test/clang-tidy/readability-simd-intrinsics.cpp

MaskRay added a comment.EditedFeb 6 2018, 2:31 PM

I haven't used clang-tidy before :) Do you have any suggestions on my workflow?

% ninja -C ~/Dev/llvm/build clangTidyReadabilityModule
% ~/Dev/llvm/build/bin/clang-tidy -checks='-*,readability-simd-intrinsics' a.cc
# for local testing

# Ensure tests are correct
% PATH=~/Dev/llvm/build/bin:$PATH test/clang-tidy/check_clang_tidy.py test/clang-tidy/readability-simd-intrinsics.cpp readability-simd-intrinsics /tmp/t

# In case I want to debug, sorry I should use lldb but ...
% cgdb -x =(printf 'set auto-solib-add off\nstart\nsha stdc\|LLVMCore.so\|Support\|clangTidy') --args ~/Dev/llvm/build/bin/clang-tidy -checks='llvm-namespace-comment' /tmp/c/a.cc

I check llvm::Triple::ArchType to reduce number of target-dependent string matching. But in the test I don't know how to enable -target ppc64le -maltivec so that I can test PPC.

MaskRay updated this revision to Diff 133091.Feb 6 2018, 3:08 PM

Move CHECK-MESSAGES: to comform to the prevaling style

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

clang-tidy/readability/SIMDIntrinsicsCheck.cpp
25

Please make function static instead of using anonymous namespace. See LLVM Coding Style.

29

I think returning {} will be better. Same for trailing return.

48

Please make function static instead of using anonymous namespace. See LLVM Coding Style.

56

I think returning {} will be better. Same for trailing return.

78

You should enable this check only when compiling in appropriate C++ version. See getLangOpts() usage in other checks.

clang-tidy/readability/SIMDIntrinsicsCheck.h
31

Please remove private section and empty line before it.

Eugene.Zelenko retitled this revision from [clang-tidy] Add `readability-simd-intrinsics` check. to [clang-tidy] Add readability-simd-intrinsics check..Feb 6 2018, 3:35 PM
Eugene.Zelenko edited reviewers, added: ilya-biryukov; removed: bkramer.
Eugene.Zelenko added a project: Restricted Project.
Eugene.Zelenko added a subscriber: bkramer.
MaskRay updated this revision to Diff 133107.Feb 6 2018, 4:09 PM
MaskRay marked 5 inline comments as done.

LLVM Style

MaskRay updated this revision to Diff 133109.Feb 6 2018, 4:10 PM

Remove private section

MaskRay marked an inline comment as done.Feb 6 2018, 4:10 PM
MaskRay added inline comments.
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
78

Thx, I didn't know getLangOpts() before.

I think even in -x c mode, :: is still the prefix of an identifier.

matchesName("^(_mm_|_mm256_|_mm512_|vec_)")

doesn't match anything but

matchesName("^::(_mm_|_mm256_|_mm512_|vec_)")

matches.

verified via clang-tidy -checks='-*,readability-simd-intrinsics' a.c -- -xc

Eugene.Zelenko added inline comments.Feb 6 2018, 4:14 PM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
78

Check should be enabled only for C++ version which has std::experimental::simd implementation, so this is why you need to use getLangOpts().

MaskRay updated this revision to Diff 133120.Feb 6 2018, 5:52 PM

if (!Result.Context->getLangOpts().CPlusPlus11) return;

MaskRay updated this revision to Diff 133126.Feb 6 2018, 6:05 PM

docs/ReleaseNotes.rst

MaskRay marked 2 inline comments as done.Feb 6 2018, 6:06 PM
Eugene.Zelenko added inline comments.Feb 6 2018, 6:11 PM
docs/ReleaseNotes.rst
103

Please move it new checks section.

timshen added inline comments.Feb 6 2018, 6:22 PM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
23

"Check" usually indicates to return a bool, but what it actually returns is a possible mapped result based on the input. Maybe some name like "TrySuggestPPC"?

timshen added inline comments.Feb 6 2018, 6:28 PM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
25

Can you either find or create a wrapper for this?

bool StripPrefix(StringRef Prefix, StringRef& S) {
  if (S.startwith(Prefix)) {
    S = S.substr(Prefix.size());
    return true;
  }
  return false;
}

Which is a huge readability boost and DRY.

timshen added inline comments.Feb 6 2018, 6:37 PM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
25

Found it: StringRef::consume_front()

timshen added inline comments.Feb 6 2018, 6:44 PM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
35

Technically, std::experimental::simd::operator+ (an all other binary operators) don't exist, as they are free functions, not member functions. operator+ for std::experimental::simd objects do exist.

MaskRay updated this revision to Diff 133133.Feb 6 2018, 6:58 PM
MaskRay marked an inline comment as done.

docs/ReleaseNotes.rst and use StringRef::consume_front

MaskRay updated this revision to Diff 133134.Feb 6 2018, 7:01 PM
MaskRay marked 2 inline comments as done.

Warning messages of operator+ as operator+ operator- .. are free functions on simd objects.

MaskRay marked 2 inline comments as done.Feb 6 2018, 7:01 PM
lebedev.ri added inline comments.
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
27

You probably want to move Mapping out of the function.

35

To point the obvious, this is not exhaustive list, at least div is missing.

56

This function was not updated to use the Mapping map.

76

Is it reasonable to suggest to use <experimental/*>?
I would guess it should be CPlusPlus2a

85

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

88

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);
}
docs/clang-tidy/checks/readability-simd-intrinsics.rst
2

This should be

.. title:: clang-tidy - readability-simd-intrinsics
4

Here too

MaskRay updated this revision to Diff 133233.Feb 7 2018, 9:05 AM
MaskRay marked 2 inline comments as done.

Fix word order of readability-simd-intrinsics

MaskRay updated this revision to Diff 133237.Feb 7 2018, 9:43 AM

Add check-specific option Experimental

StringRef Std;
if (Result.Context->getLangOpts().CPlusPlus2a) {
  Std = "std";
} else if (Result.Context->getLangOpts().CPlusPlus11 && Experimental) {
  // libcxx implementation backports it to C++11 std::experimental::simd.
  Std = "std::experimental";
} else
  return;
MaskRay updated this revision to Diff 133239.Feb 7 2018, 9:52 AM

Use unnamed namespace to enclose AST_MATCHER and TrySuggest*

MaskRay marked 2 inline comments as done.Feb 7 2018, 9:56 AM
MaskRay added inline comments.
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
76

Added a check-specific option readability-simd-intrinsics.Experiment.

85
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,
88

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?

MaskRay marked 3 inline comments as done.Feb 7 2018, 10:02 AM
MaskRay added inline comments.
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
27

Does keeping static const llvm::StringMap<std::string> Mapping{ ... } inside the function avoid a global constructor? The blacklist will surely be modified to cover more operations after the revision

35

Yes, the blacklist will be modified to cover more instructions.

Another fun fact is that though _mm_div_epi* are listed in Intel's intrinsics guide, no there are no mapping hardware instructions and the functions only exist in ICC/ICPC (I guess) not in clang/lib/Headers/*.h

56

This is because on Power, these functions are overloaded vec_add vec_sub ...
But on x86, there are suffixes, e.g. _mm_add_epi32.

lebedev.ri added inline comments.Feb 7 2018, 10:06 AM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
78

I think this is still not done,

Experimental(Options.getLocalOrGlobal("Experimental", 1) != 0)

means it will still be on by default for C++11

107

This logic should be in the beginning of registerMatchers(), so this wouldn't even be registered/called if it won't do anything.

clang-tidy/readability/SIMDIntrinsicsCheck.h
28

I think usually this is in .cpp file

MaskRay updated this revision to Diff 133255.Feb 7 2018, 10:44 AM
MaskRay marked an inline comment as done.

readability-simd-intrinsics.rst

timshen added inline comments.Feb 7 2018, 11:14 AM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
76

Is it reasonable to suggest to use <experimental/*>?

I think there are two approaches to proceed:

  1. We just warn the users, but don't suggest <experimental/simd> fixes.
  2. We warn and suggest <experimental/simd> fixes, but only when a flag is turned on (off by default). The flag documentation should clearly include "suggest <experimental/simd> API".

I'm not sure which one fits better.

lebedev.ri added inline comments.Feb 7 2018, 11:25 AM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
76

Ok, i should have commented in more words :)

I wasn't questioning the use of <experimental/simd>
I was questioning the whole approach of defaulting to issuing the warning as long as that header can be used with specified C++ standard.
E.g. there is <experimental/filesystem>, but there isn't any check to complain on fopen()/fclose() and friends.
(I do agree that this argument is chicken-or-the-egg problem)

88

I may should use unnamed namespace for AST_MATCHER

I think so.

do I also need the unnamed namespace to enclose TrySuggestPPC and TrySuggestX86?

No, but do make those static.

MaskRay updated this revision to Diff 133455.Feb 8 2018, 11:06 AM

Add option Enabled which defaults to 0.
Suggest std::simd (-std=c++2a) or std::experimental::std (-std=c++11) only if enabled.

MaskRay marked 9 inline comments as done.EditedFeb 8 2018, 11:10 AM

The check must be manually enabled now:

% clang-tidy -checks='-*,readability-simd-intrinsics' a.cc -- -std=c++2a
# Not enabled by default

% clang-tidy -checks='-*,readability-simd-intrinsics' -config='{CheckOptions: [{key: readability-simd-intrinsics.Enabled, value: 1}]}' a.cc -- -std=c++11
# warn and suggest std::experimental::simd

% clang-tidy -checks='-*,readability-simd-intrinsics' -config='{CheckOptions: [{key: readability-simd-intrinsics.Enabled, value: 1}]}' a.cc -- -std=c++2a
# warn and suggest std::simd

Current add sub mul suggestions are only POC. More checks will be added in follow-up revisions.

MaskRay updated this revision to Diff 133457.Feb 8 2018, 11:18 AM

Set Enabled to 1 in test/clang-tidy/readability-simd-intrinsics.cpp

MaskRay updated this revision to Diff 133474.Feb 8 2018, 12:50 PM

Split test/clang-tidy/readability-simd-intrinsics.cpp to x86 and ppc

lebedev.ri added inline comments.Feb 9 2018, 10:45 AM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
47

I think you can use llvm::StringRef here instead of std::string

89

Enabled seems too broad to me.
How about UseStdExperimental? (still defaults to false)

99

So sorry for beating the same stuff again, but i believe as of right now, it's still only in std::experimental namespace, not in std?

MaskRay updated this revision to Diff 133902.Feb 12 2018, 10:58 AM
MaskRay marked an inline comment as done.

Rename Enabled to UseStdExperimental and only suggest std::experimental:: (not std::)

MaskRay marked 2 inline comments as done.Feb 12 2018, 10:59 AM
hokein added inline comments.Feb 13 2018, 1:55 AM
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
47

consider using llvm::StringSwitch?

103

nit: use early return for readability.

docs/clang-tidy/checks/readability-simd-intrinsics.rst
39

We don't use check's option to enable the check in clang-tidy, if users want to enable the check, they need to pass the check explicitly to clang-tidy (e.g. -checks="<your-check>"). I'd suggest to remove this option.

MaskRay updated this revision to Diff 134076.Feb 13 2018, 10:16 AM
MaskRay marked an inline comment as done.

Remove UseStdExperimental

MaskRay marked 8 inline comments as done.Feb 13 2018, 10:23 AM
MaskRay added inline comments.
clang-tidy/readability/SIMDIntrinsicsCheck.cpp
47

The list is currently a scaffold and more operations will be added. It may be more efficient to use a lookup table instead of cascading .Case("add", ...)?

docs/clang-tidy/checks/readability-simd-intrinsics.rst
39

Removed.I don't think the option is necessary.

MaskRay updated this revision to Diff 134305.Feb 14 2018, 1:43 PM
MaskRay marked 2 inline comments as done.

Add an option Suggest.

Only suggest P0214 alternatives if it is true.

hokein accepted this revision.Feb 15 2018, 1:52 AM

The clang-tidy code looks good. If no one has further comments, feel free to commit it.

clang-tidy/readability/SIMDIntrinsicsCheck.cpp
47

I see. In your case, the normal switch-case statement seems enough? But up to you.

docs/clang-tidy/checks/readability-simd-intrinsics.rst
9

nit: I think it is stale?

test/clang-tidy/readability-simd-intrinsics-ppc.cpp
4

consider adding test when Suggest is 0?

test/clang-tidy/readability-simd-intrinsics-x86.cpp
26

nit: this is not needed. If clang-tidy gives the warning for this case, the lit test will fail.

This revision is now accepted and ready to land.Feb 15 2018, 1:52 AM
MaskRay updated this revision to Diff 134446.Feb 15 2018, 9:50 AM
MaskRay marked 4 inline comments as done.

Update .rst

MaskRay updated this revision to Diff 134447.Feb 15 2018, 9:53 AM
MaskRay marked 2 inline comments as done.

Update

MaskRay added inline comments.Feb 15 2018, 9:54 AM
test/clang-tidy/readability-simd-intrinsics-ppc.cpp
4

I'll have to duplicate the CHECK-MESSAGE lines for each intrinsic, which seems a lot of boilerplate.

This revision was automatically updated to reflect the committed changes.
alexfh added a comment.Mar 2 2018, 5:30 AM

A late comment here: should this check start a new "portability" module? This seems to be the main focus of the check rather than making code more readable.

A late comment here: should this check start a new "portability" module? This seems to be the main focus of the check rather than making code more readable.

SG. Should I rename it?

alexfh added a comment.Mar 6 2018, 3:11 AM

A late comment here: should this check start a new "portability" module? This seems to be the main focus of the check rather than making code more readable.

SG. Should I rename it?

If nobody objects, yes, please. You'll need create the module manually and then use the rename_check.py script for the actual renaming.

A late comment here: should this check start a new "portability" module? This seems to be the main focus of the check rather than making code more readable.

SG. Should I rename it?

If nobody objects, yes, please. You'll need create the module manually and then use the rename_check.py script for the actual renaming.

I just created D44173 . Didn't know rename_check.py and manually renamed a bunch of files...