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

[Fangrui Song via Phabricator via cfe-commits]

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


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:26
+
+  static const llvm::StringMap<std::string> Mapping{
+    // [simd.alg]
----------------
lebedev.ri wrote:
> You probably want to move `Mapping` out of the function.
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 


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:34
+    {"sub", "operator- on std::experimental::simd objects"},
+    {"mul", "operator* on std::experimental::simd objects"},
+  };
----------------
lebedev.ri wrote:
> To point the obvious, this is not exhaustive list, at least `div` is missing.
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


================
Comment at: clang-tidy/readability/SIMDIntrinsicsCheck.cpp:55
+  // [simd.binary]
+  if (Name.startswith("add_"))
+    return "operator+ on std::experimental::simd objects";
----------------
lebedev.ri wrote:
> This function was not updated to use the `Mapping` map.
This is because on Power, these functions are overloaded `vec_add` `vec_sub` ...
But on x86, there are suffixes, e.g. `_mm_add_epi32`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42983