cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.07k stars 4.28k forks source link

Updating LLVM to version 18.1 for CMSSW_14_2_X #45870

Open smuzaffar opened 1 week ago

smuzaffar commented 1 week ago

@cms-sw/all-l2 , we are going to update llvm to version 18.1.6 for CMSSW_14_2_X. The new clang-format in LLVM 18.1 proposes couple of new changes which were not handled properly by llvm 17.

  1. extra space for inline function: (){} -> () {}: clang-format in llvm 17 was only adding the space it there is no ; after the function definition e.g. for https://github.com/cms-sw/cmssw/blob/master/DataFormats/Math/interface/Graph.h#L121-L122

    Graph() : edges_(1) {}
    ~Graph() {}

    llvm 17 was adding the extra space but not for https://github.com/cms-sw/cmssw/blob/master/Alignment/CocoaDDLObjects/interface/CocoaMaterialElementary.h#L20

    ~CocoaMaterialElementary(){};

    llvm 18 has fixed this and now clang-format properly handle this case.

  2. Comments alignment: new clang-format also suggest changes like

    --- a/Alignment/CommonAlignmentMonitor/plugins/AlignmentMonitorMuonSystemMap1D.cc
    +++ b/Alignment/CommonAlignmentMonitor/plugins/AlignmentMonitorMuonSystemMap1D.cc
    @@ -276,7 +276,7 @@ void AlignmentMonitorMuonSystemMap1D::book() {
           }
         }
       }  // endcaps
    -  }      // stations
    +  }  // stations
    
    m_counter_event = 0;
    m_counter_track = 0;
    @@ -322,7 +322,7 @@ void AlignmentMonitorMuonSystemMap1D::event(const edm::Event &iEvent,
           processMuonResidualsFromTrack(muonResidualsFromTrack, iEvent);
         }
       }  // end if track has acceptable momentum
    -    }    // end loop over tracks
    +    }  // end loop over tracks
    } else {
     const edm::Handle<reco::MuonCollection> &muons = iEvent.getHandle(muonToken_);

List of all clang-format changes (2148 files in total) are available here. Once llvm 18 is part of 14.2.X IBs then @cms-sw/core-l2 team will open code format updates PRs for files which are not already touched by any existing PR. Note that these changes llvm18 based code-format changes should be done once llvm 18 is in IBs otherwise PR code-checks using llvm17 will suggest to revert these changes.

Let us know if you have any concerns

cmsbuild commented 1 week ago

cms-bot internal usage

cmsbuild commented 1 week ago

A new Issue was created by @smuzaffar.

@Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

fwyzard commented 1 week ago
~CocoaMaterialElementary(){};

Shouldn't the trailing ; be removed ?

fwyzard commented 1 week ago

There are some other kind of changes:

diff --git a/HeterogeneousCore/AlpakaInterface/interface/CachedBufAlloc.h b/HeterogeneousCore/AlpakaInterface/interface/CachedBufAlloc.h
index 36aa8ec079b..64b518e02c2 100644
--- a/HeterogeneousCore/AlpakaInterface/interface/CachedBufAlloc.h
+++ b/HeterogeneousCore/AlpakaInterface/interface/CachedBufAlloc.h
@@ -26,8 +26,9 @@ namespace cms::alpakatools {
     template <typename TElem, typename TDim, typename TIdx, typename TQueue>
     struct CachedBufAlloc<TElem, TDim, TIdx, alpaka::DevCpu, TQueue, void> {
       template <typename TExtent>
-      ALPAKA_FN_HOST static auto allocCachedBuf(alpaka::DevCpu const& dev, TQueue queue, TExtent const& extent)
-          -> alpaka::BufCpu<TElem, TDim, TIdx> {
+      ALPAKA_FN_HOST static auto allocCachedBuf(alpaka::DevCpu const& dev,
+                                                TQueue queue,
+                                                TExtent const& extent) -> alpaka::BufCpu<TElem, TDim, TIdx> {
         // non-cached, queue-ordered asynchronous host-only memory
         return alpaka::allocAsyncBuf<TElem, TIdx>(queue, extent);
       }

(I don't mind, just pointing it out)

smuzaffar commented 1 week ago

ah right, sorry I missed that one. The change in https://github.com/cms-sw/cmssw/issues/45870#issuecomment-2328848346 is also only suggested by llvm18 and llvm17 will revert it

smuzaffar commented 1 week ago
~CocoaMaterialElementary(){};

Shouldn't the trailing ; be removed ?

yes, I agree

smuzaffar commented 1 week ago

There is RemoveSemicolon: true (currently set to false for cmssw via Google code Style) which can remove the trailing ; but looks like it is not working properly in llvm18 (or earlier releases). This has been fixed for llvm19 (not yet released though)

makortel commented 1 week ago

assign core

cmsbuild commented 1 week ago

New categories assigned: core

@Dr15Jones,@makortel,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks

tomalin commented 4 days ago

@smuzaffar for development work in a personal branch based on a branch of 14_1, this PR causes a lot of git conflicts, as LLVM 18.1 changes many lines of code. Can you advise how people can run the LLVM 18.1 corrections on their personal 14_1 branches, before they make a PR to master, to avoid this?

smuzaffar commented 4 days ago

@tomalin , first I would suggest to move the development to 14.2.X if changes are to be included in master branch. If it is not possible or not easy then you can try the following ( make sure to make a copy of your existing 14.1.X src tree)

#create a CMSSW_14_2_X IB area  and set env
scram project CMSSW_14_2_X_2024-09-10-2300
cd CMSSW_14_2_X_2024-09-10-2300
cmsenv

# go to your 14.1.X dev area and run clang-format for all changed files e.g. if it is CMSSW_14_1_X_2024-09-10-2300
cd CMSSW_14_1_X_2024-09-10-2300
cd src
git diff --name-only CMSSW_14_1_X_2024-09-10-2300 | xargs clang-format -i 

Note that there are some manual changes required too e.g. removing the ; (see https://github.com/cms-sw/cmssw/issues/45870#issuecomment-2329100580)