[NFC] Apply clang-tidy (#90)

This applies a set of clang-tidy rules to enforce LLVM coding standards
and some common cleanups.

This PR configures clang-tidy to enforce a swath of rules that comply with and enforce LLVM's coding standards.

Two new CMake options are added:

OFFLOADTEST_USE_CLANG_TIDY - When set to On this runs clang-tidy on non-third-party sources in the offload-test-suite as part of the build process.
OFFLOADTEST_CLANG_TIDY_APPLY_FIX - When set to On passes --fix to clang-tidy to automatically apply fix-its for any issues that have fixes available (Requires OFFLOADTEST_USE_CLANG_TIDY=On).
The enabled warning set is:

clang-diagnostic-* - Clang compiler diagnostics.
llvm-* - LLVM coding standards
misc-* - All miscellaneous rules except:

misc-use-anonymous-namespace, which violates LLVM coding standards which prefers static for functions.
misc-include-cleaner, which did some unholy things to a few source files and caused them to fail to compile.
*misc-non-private-member-variables-in-classes, which conflicts with common patterns in LLVM code.
readability-identifier-naming - Apply LLVM naming conventions

Additionally this PR adds the QualifierAlignment: Left option to the .clang-format configuration because the clang-tidy misc-const-correctness rule adds const to the right of the type which is less common in every codebase ever (Thank's Clang!).

Author: llvm-beanz

.clang-format

@@ -1 +1,2 @@
 BasedOnStyle: LLVM
+QualifierAlignment: Left

.clang-tidy

@@ -0,0 +1,20 @@
+Checks: '-*,clang-diagnostic-*,llvm-*,misc-*,-misc-use-anonymous-namespace,-misc-include-cleaner,-misc-non-private-member-variables-in-classes,readability-identifier-naming'
+WarningsAsErrors: '*'
+CheckOptions:
+  - key:             readability-identifier-naming.ClassCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.EnumCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.FunctionCase
+    value:           camelBack
+  - key:             readability-identifier-naming.MemberCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.ParameterCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.UnionCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.VariableCase
+    value:           CamelCase
+  - key:             readability-identifier-naming.IgnoreMainLikeFunctions
+    value:           1
+ExcludeHeaderFilterRegex: ".*third-party.*"

.github/workflows/windows-intel-clang-d3d12.yaml

@@ -22,4 +22,4 @@ jobs:
       Test-Clang: On
       TestTarget: check-hlsl-clang-d3d12
       OffloadTest-branch: ${{ github.event.pull_request.head.sha || github.ref }}
-      LLVM-ExtraCMakeArgs: -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl
+      LLVM-ExtraCMakeArgs: -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl -DOFFLOADTEST_USE_CLANG_TIDY=On

.github/workflows/windows-intel-clang-vk.yaml

@@ -22,4 +22,4 @@ jobs:
       Test-Clang: On
       TestTarget: check-hlsl-clang-vk
       OffloadTest-branch: ${{ github.event.pull_request.head.sha || github.ref }}
-      LLVM-ExtraCMakeArgs: -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl
+      LLVM-ExtraCMakeArgs: -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl -DOFFLOADTEST_USE_CLANG_TIDY=On

.github/workflows/windows-intel-clang-warp-d3d12.yaml

@@ -22,4 +22,4 @@ jobs:
       Test-Clang: On
       TestTarget: check-hlsl-clang-warp-d3d12
       OffloadTest-branch: ${{ github.event.pull_request.head.sha || github.ref }}
-      LLVM-ExtraCMakeArgs: -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl
+      LLVM-ExtraCMakeArgs: -DCMAKE_C_COMPILER=clang-cl -DCMAKE_CXX_COMPILER=clang-cl -DOFFLOADTEST_USE_CLANG_TIDY=On

CMakeLists.txt

@@ -96,6 +96,37 @@ set_property(GLOBAL APPEND PROPERTY LLVM_EXPORTS_BUILDTREE_ONLY png_static)
 
 include(Warp)
 
+# This must be after the third-party targets are generated so that we only apply
+# it to code in the offload-test-suite.
+option(OFFLOADTEST_USE_CLANG_TIDY "Use clang-tidy on offload test codebase" Off)
+option(OFFLOADTEST_CLANG_TIDY_APPLY_FIX "Automatically apply clang-tidy fixes" Off)
+
+if (OFFLOADTEST_USE_CLANG_TIDY)
+  find_program(CLANG_TIDY clang-tidy)
+  if (NOT CLANG_TIDY)
+    message(
+      FATAL_ERROR
+        "Clang-tidy not found in PATH, please specify CLANG_TIDY to CMake.")
+  endif ()
+  if (APPLE)
+    if (CMAKE_OSX_SYSROOT)
+      set(CLANG_TIDY_ARGS --extra-arg=-isysroot${CMAKE_OSX_SYSROOT})
+    elseif(NOT CMAKE_CROSSCOMPILING)
+      # xcrun -sdk macosx --show-sdk-path
+      execute_process(COMMAND xcrun -sdk macosx --show-sdk-path
+        OUTPUT_VARIABLE SYSROOT
+        ERROR_QUIET
+        OUTPUT_STRIP_TRAILING_WHITESPACE)
+      set(CLANG_TIDY_ARGS --extra-arg=-isysroot${SYSROOT})
+    endif()
+  endif()
+  if (OFFLOADTEST_CLANG_TIDY_APPLY_FIX)
+    set(CLANG_TIDY_ARGS ${CLANG_TIDY_ARGS} --fix)
+  endif ()
+  set(CMAKE_C_CLANG_TIDY ${CLANG_TIDY} ${CLANG_TIDY_ARGS})
+  set(CMAKE_CXX_CLANG_TIDY ${CLANG_TIDY} ${CLANG_TIDY_ARGS})
+endif ()
+
 add_subdirectory(lib)
 add_subdirectory(tools)
 

README.md

@@ -36,6 +36,12 @@ compiler to use by passing:
 -DDXC_DIR=<path to folder containing dxc & dxv>
 ```
 
+## Enabling clang-tidy
+
+The offload test suite's code is clang-tidy clean for a limited ruleset.
+If you have clang-tidy installed locally you can enable clang-tidy by adding `-DOFFLOADTEST_USE_CLANG_TIDY=On` to your CMake invocation.
+You can also add `-DOFFLOADTEST_CLANG_TIDY_APPLY_FIX=On` to enable automatically applying the clang-tidy fix-its for any warnings that have automated fixes.
+
 # YAML Pipeline Format
 
 This framework provides a YAML representation for describing GPU pipelines and buffers. The format is implemented by the `API/Pipeline.{h|cpp}` sources. The following is an example pipeline YAML description:

docs/WSL.md

@@ -20,10 +20,10 @@ Alternatively, build and install the DirectX-Headers from the [original repo](ht
   - Fix: Use a newer version of DXC that includes the validator binary `dxv`
 - `double free or corruption (!prev)` may occur when `api-query` and `offloader` exit, which then follows with an infinite stall instead of program termination
   - A workaround is implemented which avoids this issue on normal program exit (i.e., `return 0;` from main)
-  - This issue appears to occur whenever `DeviceContext::Devices` from `lib/API/Device.cpp` contains more than one device. 
-    When `api-query` or `offloader` exit, some memory resources are failing to automatically free themselves. 
-    Valgrind output suggests the core issue stems from the implementation of `Microsoft::WRL::ComPtr::InternalRelease` in WSL. 
+  - This issue appears to occur whenever `DeviceContext::Devices` from `lib/API/Device.cpp` contains more than one device.
+    When `api-query` or `offloader` exit, some memory resources are failing to automatically free themselves.
+    Valgrind output suggests the core issue stems from the implementation of `Microsoft::WRL::ComPtr::InternalRelease` in WSL.
     Manually clearing `DeviceContext::Devices` before a program exit appears to eliminate the issue for the exit
-  - An additional workaround that would cover all possible program exit points is to only select one `DXDevice` by editing `lib/API/DX/Device.cpp` to `break;` at the end of the first iteration of the for-loop in the function `InitializeDXDevices()`
+  - An additional workaround that would cover all possible program exit points is to only select one `DXDevice` by editing `lib/API/DX/Device.cpp` to `break;` at the end of the first iteration of the for-loop in the function `initializeDXDevices()`
     - Before implementing this workaround, you can run `api-query` to list all GPU devices available. Afterwards, edit the `AdapterIndex` of the for-loop to select the index of the desired GPU
 

include/API/Device.h

@@ -12,6 +12,8 @@
 #ifndef OFFLOADTEST_API_DEVICE_H
 #define OFFLOADTEST_API_DEVICE_H
 
+#include "Config.h"
+
 #include "API/API.h"
 #include "API/Capabilities.h"
 #include "llvm/ADT/StringRef.h"
@@ -54,6 +56,18 @@ class Device {
   static DeviceIterator begin();
   static DeviceIterator end();
   static inline DeviceRange devices() { return DeviceRange(begin(), end()); }
+
+#ifdef OFFLOADTEST_ENABLE_D3D12
+  static llvm::Error initializeDXDevices();
+#endif
+
+#ifdef OFFLOADTEST_ENABLE_VULKAN
+  static llvm::Error initializeVXDevices();
+#endif
+
+#ifdef OFFLOADTEST_ENABLE_METAL
+  static llvm::Error initializeMtlDevices();
+#endif
 };
 
 } // namespace offloadtest

lib/API/DX/Device.cpp

@@ -158,7 +158,7 @@ class DXDevice : public offloadtest::Device {
   llvm::StringRef getAPIName() const override { return "DirectX"; }
   GPUAPI getAPI() const override { return GPUAPI::DirectX; }
 
-  static llvm::Expected<DXDevice> Create(ComPtr<IDXCoreAdapter> Adapter) {
+  static llvm::Expected<DXDevice> create(ComPtr<IDXCoreAdapter> Adapter) {
     ComPtr<ID3D12Device> Device;
     if (auto Err =
             HR::toError(D3D12CreateDevice(Adapter.Get(), D3D_FEATURE_LEVEL_11_0,
@@ -234,7 +234,7 @@ class DXDevice : public offloadtest::Device {
         HasRootSigPart = true;
 
     if (HasRootSigPart) {
-      llvm::StringRef Binary = P.Shaders[0].Shader->getBuffer();
+      const llvm::StringRef Binary = P.Shaders[0].Shader->getBuffer();
       if (auto Err = HR::toError(
               Device->CreateRootSignature(0, Binary.data(), Binary.size(),
                                           IID_PPV_ARGS(&State.RootSig)),
@@ -244,15 +244,15 @@ class DXDevice : public offloadtest::Device {
     }
 
     std::vector<D3D12_ROOT_PARAMETER> RootParams;
-    uint32_t DescriptorCount = P.getDescriptorCount();
-    std::unique_ptr<D3D12_DESCRIPTOR_RANGE[]> Ranges =
+    const uint32_t DescriptorCount = P.getDescriptorCount();
+    const std::unique_ptr<D3D12_DESCRIPTOR_RANGE[]> Ranges =
         std::unique_ptr<D3D12_DESCRIPTOR_RANGE[]>(
             new D3D12_DESCRIPTOR_RANGE[DescriptorCount]);
 
     uint32_t RangeIdx = 0;
     for (const auto &D : P.Sets) {
       uint32_t DescriptorIdx = 0;
-      uint32_t StartRangeIdx = RangeIdx;
+      const uint32_t StartRangeIdx = RangeIdx;
       for (const auto &R : D.Resources) {
         switch (getDXKind(R.Kind)) {
         case SRV:
@@ -291,7 +291,7 @@ class DXDevice : public offloadtest::Device {
             D3D12SerializeRootSignature(&Desc, D3D_ROOT_SIGNATURE_VERSION_1,
                                         &Signature, &Error),
             "Failed to seialize root signature.")) {
-      std::string Msg =
+      const std::string Msg =
           std::string(reinterpret_cast<char *>(Error->GetBufferPointer()),
                       Error->GetBufferSize() / sizeof(char));
       return joinErrors(
@@ -322,8 +322,7 @@ class DXDevice : public offloadtest::Device {
     return llvm::Error::success();
   }
 
-  llvm::Error createPSO(Pipeline &P, llvm::StringRef DXIL,
-                        InvocationState &State) {
+  llvm::Error createPSO(llvm::StringRef DXIL, InvocationState &State) {
     const D3D12_COMPUTE_PIPELINE_STATE_DESC Desc = {
         State.RootSig.Get(),
         {DXIL.data(), DXIL.size()},
@@ -428,7 +427,7 @@ class DXDevice : public offloadtest::Device {
                ComPtr<ID3D12Resource> Buffer) {
     const uint32_t EltSize = R.getElementSize();
     const uint32_t NumElts = R.size() / EltSize;
-    DXGI_FORMAT EltFormat =
+    DXGI_FORMAT const EltFormat =
         R.isRaw() ? getRawDXFormat(R)
                   : getDXFormat(R.BufferPtr->Format, R.BufferPtr->Channels);
     const D3D12_SHADER_RESOURCE_VIEW_DESC SRVDesc = {
@@ -530,7 +529,7 @@ class DXDevice : public offloadtest::Device {
                ComPtr<ID3D12Resource> Buffer) {
     const uint32_t EltSize = R.getElementSize();
     const uint32_t NumElts = R.size() / EltSize;
-    DXGI_FORMAT EltFormat =
+    DXGI_FORMAT const EltFormat =
         R.isRaw() ? getRawDXFormat(R)
                   : getDXFormat(R.BufferPtr->Format, R.BufferPtr->Channels);
     const D3D12_UNORDERED_ACCESS_VIEW_DESC UAVDesc = {
@@ -556,7 +555,7 @@ class DXDevice : public offloadtest::Device {
   }
 
   llvm::Expected<ResourceSet> createCBV(Resource &R, InvocationState &IS) {
-    size_t CBVSize = getCBVSize(R.size());
+    const size_t CBVSize = getCBVSize(R.size());
     llvm::outs() << "Creating CBV: { Size = " << CBVSize << ", Register = b"
                  << R.DXBinding.Register << ", Space = " << R.DXBinding.Space
                  << " }\n";
@@ -617,7 +616,7 @@ class DXDevice : public offloadtest::Device {
 
   void bindCBV(Resource &R, InvocationState &IS, const uint32_t HeapIdx,
                ComPtr<ID3D12Resource> Buffer) {
-    size_t CBVSize = getCBVSize(R.size());
+    const size_t CBVSize = getCBVSize(R.size());
     const D3D12_CONSTANT_BUFFER_VIEW_DESC CBVDesc = {
         Buffer->GetGPUVirtualAddress(), static_cast<uint32_t>(CBVSize)};
 
@@ -754,7 +753,7 @@ class DXDevice : public offloadtest::Device {
   llvm::Error waitForSignal(InvocationState &IS) {
     // This is a hack but it works since this is all single threaded code.
     static uint64_t FenceCounter = 0;
-    uint64_t CurrentCounter = FenceCounter + 1;
+    const uint64_t CurrentCounter = FenceCounter + 1;
 
     if (auto Err = HR::toError(IS.Queue->Signal(IS.Fence.Get(), CurrentCounter),
                                "Failed to add signal."))
@@ -802,7 +801,7 @@ class DXDevice : public offloadtest::Device {
     IS.CmdList->SetPipelineState(IS.PSO.Get());
     IS.CmdList->SetComputeRootSignature(IS.RootSig.Get());
 
-    uint32_t Inc = Device->GetDescriptorHandleIncrementSize(
+    const uint32_t Inc = Device->GetDescriptorHandleIncrementSize(
         D3D12_DESCRIPTOR_HEAP_TYPE_CBV_SRV_UAV);
     CD3DX12_GPU_DESCRIPTOR_HANDLE Handle;
 
@@ -816,12 +815,13 @@ class DXDevice : public offloadtest::Device {
       uint32_t ConstantOffset = 0u;
       uint32_t RootParamIndex = 0u;
       uint32_t DescriptorTableIndex = 0u;
-      auto RootDescIt = IS.RootResources.begin();
+      auto *RootDescIt = IS.RootResources.begin();
       for (const auto &Param : P.Settings.DX.RootParams) {
         switch (Param.Kind) {
         case dx::RootParamKind::Constant: {
           auto &Constant = std::get<dx::RootConstant>(Param.Data);
-          uint32_t NumValues = Constant.BufferPtr->size() / sizeof(uint32_t);
+          const uint32_t NumValues =
+              Constant.BufferPtr->size() / sizeof(uint32_t);
           IS.CmdList->SetComputeRoot32BitConstants(
               RootParamIndex++, NumValues, Constant.BufferPtr->Data.get(),
               ConstantOffset);
@@ -865,7 +865,7 @@ class DXDevice : public offloadtest::Device {
       }
     }
 
-    llvm::ArrayRef<int> DispatchSize =
+    const llvm::ArrayRef<int> DispatchSize =
         llvm::ArrayRef<int>(P.Shaders[0].DispatchSize);
 
     IS.CmdList->Dispatch(DispatchSize[0], DispatchSize[1], DispatchSize[2]);
@@ -886,7 +886,7 @@ class DXDevice : public offloadtest::Device {
       CopyBackResource(R);
   }
 
-  llvm::Error readBack(Pipeline &P, InvocationState &IS) {
+  llvm::Error readBack(InvocationState &IS) {
     auto MemCpyBack = [](ResourcePair &R) -> llvm::Error {
       if (!R.first->isReadWrite())
         return llvm::Error::success();
@@ -946,7 +946,7 @@ class DXDevice : public offloadtest::Device {
     if (auto Err = createDescriptorHeap(P, State))
       return Err;
     llvm::outs() << "Descriptor heap created.\n";
-    if (auto Err = createPSO(P, P.Shaders[0].Shader->getBuffer(), State))
+    if (auto Err = createPSO(P.Shaders[0].Shader->getBuffer(), State))
       return Err;
     llvm::outs() << "PSO created.\n";
     if (auto Err = createCommandStructures(State))
@@ -963,7 +963,7 @@ class DXDevice : public offloadtest::Device {
     if (auto Err = executeCommandList(State))
       return Err;
     llvm::outs() << "Compute commands executed.\n";
-    if (auto Err = readBack(P, State))
+    if (auto Err = readBack(State))
       return Err;
     llvm::outs() << "Read data back.\n";
 
@@ -972,7 +972,7 @@ class DXDevice : public offloadtest::Device {
 };
 } // namespace
 
-llvm::Error InitializeDXDevices() {
+llvm::Error Device::initializeDXDevices() {
 #ifdef _WIN32
 #ifndef NDEBUG
   ComPtr<ID3D12Debug1> Debug1;
@@ -1010,7 +1010,7 @@ llvm::Error InitializeDXDevices() {
             "Failed to acquire adapter")) {
       return Err;
     }
-    auto ExDevice = DXDevice::Create(Adapter);
+    auto ExDevice = DXDevice::create(Adapter);
     if (!ExDevice)
       return ExDevice.takeError();
     auto ShPtr = std::make_shared<DXDevice>(*ExDevice);