oneMKL icon indicating copy to clipboard operation
oneMKL copied to clipboard

[clang-format] Reformat due to transition from v9.0.0 to v19.1.0 and update of style configuration

Open dnhsieh-intel opened this issue 1 year ago • 4 comments

Description

This PR is a preparation step for issue #580. Since we will move from clang-format v9.0.0 to v19.1.0, this PR reformats the code according to v19.1.0. This PR is separated from the CI enabling (PR #595) in order to have a more informative git blame.

There are some changes I want to discuss.

  • src/lapack/backends/cusolver/cusolver_lapack.cpp The following changes are suggested by clang-format:
    -    queue.submit([&](sycl::handler &cgh) {
    -        onemkl_cusolver_host_task(cgh, queue, [=](CusolverScopedContextHandler &sc) {
    -            auto handle = sc.get_handle(queue);
    -            cusolverStatus_t err;
    -            CUSOLVER_ERROR_FUNC_T_SYNC(func_name, func, err, handle, m, n, scratch_size);
    -        });
    -    }).wait();
    +    queue
    +        .submit([&](sycl::handler &cgh) {
    +            onemkl_cusolver_host_task(cgh, queue, [=](CusolverScopedContextHandler &sc) {
    +                auto handle = sc.get_handle(queue);
    +                cusolverStatus_t err;
    +                CUSOLVER_ERROR_FUNC_T_SYNC(func_name, func, err, handle, m, n, scratch_size);
    +            });
    +        })
    +        .wait();
    
    Because it looks too funny, I added // clang-format off/on for all the similar suggestions in this file.
  • tests/unit_tests/dft/include/{compute_out_of_place,compute_inplace}.hpp Although I don't like the following formatting personally, it is probably still fine. I can turn the formatting off if there are objections.
    -                EXPECT_TRUE(check_equal_strided<domain == oneapi::mkl::dft::domain::REAL>(
    -                    bwd_ptr + backward_distance * i, out_host_ref.data() + ref_distance * i, sizes,
    -                    strides_bwd_cpy, abs_error_margin, rel_error_margin, std::cout));
    +                EXPECT_TRUE(check_equal_strided < domain ==
    +                            oneapi::mkl::dft::domain::REAL >
    +                                (bwd_ptr + backward_distance * i,
    +                                 out_host_ref.data() + ref_distance * i, sizes, strides_bwd_cpy,
    +                                 abs_error_margin, rel_error_margin, std::cout));
    

Checklist

All Submissions

  • Do all unit tests pass locally? Only code reformatting. The CPU tests in CI passed.
  • [X] Have you formatted the code using clang-format?

dnhsieh-intel avatar Oct 15 '24 07:10 dnhsieh-intel

The diff looks good to me overall. Regarding the changes in src/lapack/backends/cusolver/cusolver_lapack.cpp, could we try to change the clang-format configuration file to get a good enough formatting without // clang-format off/on? Looking at the repository's _clang-format and the documentation there are a bunch of deprecated configurations that we should update:

  • Standard: Cpp11 -> Standard: c++17 (or Standard: Latest)
  • AllowAllConstructorInitializersOnNextLine and ConstructorInitializerAllOnOneLineOrOnePerLine can be replaced with PackConstructorInitializers: Never
  • AlwaysBreakAfterDefinitionReturnType can be removed
  • AlwaysBreakAfterReturnType -> BreakAfterReturnType: Automatic
  • AlwaysBreakTemplateDeclarations -> BreakTemplateDeclarations: Yes
  • KeepEmptyLinesAtTheStartOfBlocks ->
    KeepEmptyLines:
      AtEndOfFile: true
      AtStartOfBlock: false
      AtStartOfFile: false
    
  • SpaceInEmptyParentheses -> SpacesInParens: Never

Maybe this will be enough to fix the issue. If not we could maybe add the configuration PenaltyIndentedWhitespace: 500 for instance.

Rbiessy avatar Oct 15 '24 13:10 Rbiessy

I agree with @Rbiessy that we should try to improve the configuration file rather than using // clang-format off. This PR is a good opportunity to improve the config file.

andrewtbarker avatar Oct 15 '24 15:10 andrewtbarker

It looks like this is just how clang-format handles method chaining:

    queue
        .submit([&](sycl::handler& cgh) {
            // body
            });
        })
        .wait();

I didn't find an option for this unfortunately. One workaround is to write queue.submit() and queue.wait() separately, but this is against the purpose of using automatic formatting. I think we just let clang-format do its job...

The style configuration file is updated in the commit ff81d95. Most changes and clean-up are straightforward. However, one significant change is DerivePointerAlignment: false. The description of this option is

If true, analyze the formatted file for the most common alignment of & and *. Pointer and reference alignment styles are going to be updated according to the preferences found in the file. PointerAlignment is then used only as fallback.

Because the alignment style is according to the preferences found in the file, the current code base has both left and right alignments, for example, https://github.com/oneapi-src/oneMKL/blob/6923d402d5bccba9ae1966062bc5a277fc74776c/examples/blas/run_time_dispatching/level3/gemm_usm.cpp#L63 https://github.com/oneapi-src/oneMKL/blob/6923d402d5bccba9ae1966062bc5a277fc74776c/examples/blas/compile_time_dispatching/level3/gemm_usm_mklcpu_cublas.cpp#L64 I am open to either left or right alignment, but we must choose one in my opinion.

dnhsieh-intel avatar Oct 18 '24 07:10 dnhsieh-intel

I didn't mean to approve on behalf of the other teams, I don't know of way to change this in GitHub so I have just requested for their approval again!

Rbiessy avatar Oct 18 '24 09:10 Rbiessy

@oneapi-src/onemkl-rng-write, @oneapi-src/onemkl-sparse-write, @oneapi-src/onemkl-dft-write I plan to merge the PR on 10/22 EOD Pacific time if there are no objections.

dnhsieh-intel avatar Oct 21 '24 19:10 dnhsieh-intel