Comments to #41285

[AdrianoDee]

This issue keeps track of the comment addressed for the PRs:

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285

---

15 November 2023

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/CalibTracker/SiPixelESProducers/plugins/BuildFile.xml

  Line 34 in 1909d90

  <use name="RecoTracker/Record"/>

  
  You could leave all the common dependencies outside of the <library> element. Those dependencies will apply to all declared plugins.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/CalibTracker/SiPixelESProducers/plugins/alpaka/SiPixelCablingSoAESProducer.cc

  Line 46 in 1909d90

  desc.add<std::string>("appendToDataLabel", "");

  • Suggestion: No need, framework does it for you.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/CalibTracker/SiPixelESProducers/plugins/alpaka/SiPixelCablingSoAESProducer.cc

  Lines 35 to 36 in 1909d90

  	auto const& component = iConfig.getParameter<std::string>("appendToDataLabel");
  	auto cc = setWhatProduced(this, component);

  • Change with:
    Don't do this with the appendToDataLabel, framework will do the same automatically for you. (on the top of my head I'd guess this way the appendToDataLabel would get applied twice)

         auto cc = setWhatProduced(this);

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/CalibTracker/SiPixelESProducers/plugins/alpaka/SiPixelCablingSoAESProducer.cc

  Line 59 in 1909d90

  bool hasQuality = quality != nullptr;

  
  The hasQuality is equivalent to useQuality_, so is it really needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/CalibTracker/SiPixelESProducers/plugins/alpaka/SiPixelCablingSoAESProducer.cc

  Line 143 in 1909d90

  bool useQuality_;

  • Change with:

       const bool useQuality_;

  ?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/Configuration/ProcessModifiers/python/alpaka_cff.py

  Line 5 in 1909d90

  alpaka = cms.Modifier()

  
  This was included in Add Alpaka Process Modifier cms-sw/cmssw#43229 that is now merged.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/BuildFile.xml

  Line 1 in 1909d90

  <use name="alpaka"/>

  
  Is the new package really needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/BuildFile.xml

  Line 6 in 1909d90

  <flags ALPAKA_BACKENDS="cuda rocm"/>

  • Change with:

   <flags ALPAKA_BACKENDS="!serial"/>

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/interface/ClusteringConstants.h

  Line 6 in 1909d90

  //TODO: move this to TrackerTraits!

  
  Is this TODO still relevant?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/interface/alpaka/SiPixelClustersCollection.h

  Lines 16 to 20 in 1909d90

  	#ifdef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED
  	using SiPixelClustersCollection = SiPixelClustersHost;
  	#else
  	using SiPixelClustersCollection = SiPixelClustersDevice<Device>;
  	#endif

  • Change with:
    How about

     using SiPixelClustersCollection = std::conditional_t<std::is_same_v<Device, alpaka::DevCpu>, SiPixelClustersHost, SiPixelClustersDevice<Device>>;

  ?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/src/classes_def.xml

  Lines 5 to 15 in 1909d90

  	<read
  	sourceClass="PortableHostCollection<SiPixelClustersSoA>"
  	targetClass="PortableHostCollection<SiPixelClustersSoA>"
  	version="[1-]"
  	source="SiPixelClustersSoA layout_;"
  	target="buffer_"
  	embed="false">
  	<![CDATA[
  	PortableHostCollection<SiPixelClustersSoA>::ROOTReadStreamer(newObj, onfile.layout_);
  	]]>
  	</read>

  
  Replace with

   SET_PORTABLEHOSTCOLLECTION_READ_RULES(PortableHostCollection<SiPixelClustersSoA>);

  in classes.cc?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/test/BuildFile.xml

  Line 5 in 1909d90

  <flags ALPAKA_BACKENDS="1"/>

  • Change with:
    Inconsistent indentation

     <flags ALPAKA_BACKENDS="1"/>

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/test/alpaka/Clusters_test.cc

  Line 2 in 1909d90

  #include <unistd.h>

  
  Why is unistd.h needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/interface/alpaka/SiPixelClustersCollection.h

  Line 4 in 1909d90

  #include <cstdint>

  
  cstdint does not seem to be needed in this header.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/interface/alpaka/SiPixelClustersCollection.h

  Line 35 in 1909d90
  • Change with:
    How about adding

   
   ASSERT_DEVICE_MATCHES_HOST_COLLECTION(SiPixelClustersCollection, SiPixelClustersHost);

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelClusterSoA/interface/alpaka/SiPixelClustersCollection.h

  Lines 24 to 27 in 1909d90

  	template <>
  	struct CopyToHost<ALPAKA_ACCELERATOR_NAMESPACE::SiPixelClustersCollection> {
  	template <typename TQueue>
  	static auto copyAsync(TQueue &queue, ALPAKA_ACCELERATOR_NAMESPACE::SiPixelClustersCollection const &srcData) {

  • Change with:

     template <typename TDevice>
     struct CopyToHost<SiPixelClustersDevice<TDevice>> {
       template <typename TQueue>
       static auto copyAsync(TQueue &queue, SiPixelClustersDevice<TDevice> const &srcData) {

  would be better (avoids defining a specialization for the copy from SiPixelClustersHost to itself)

---

16 November 2023

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelDigiSoA/interface/SiPixelDigisSoAv2.h

  Line 1 in b4dede0

  #ifndef DataFormats_SiPixelDigiSoA_interface_SiPixelDigisSoAv2_h

  
  Why v2?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelDigiSoA/interface/alpaka/SiPixelDigiErrorsCollection.h

  Lines 23 to 26 in b4dede0

  	template <>
  	struct CopyToHost<ALPAKA_ACCELERATOR_NAMESPACE::SiPixelDigiErrorsCollection> {
  	template <typename TQueue>
  	static auto copyAsync(TQueue& queue, ALPAKA_ACCELERATOR_NAMESPACE::SiPixelDigiErrorsCollection const& srcData) {

  • Change with:
    Also here

     template <typename TDevice>
     struct CopyToHost<SiPixelDigiErrorsDevice<TDevice>> {
       template <typename TQueue>
       static auto copyAsync(TQueue& queue, SiPixelDigiErrorsDevice<TDevice> const& srcData) {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelDigiSoA/interface/alpaka/SiPixelDigiErrorsUtilities.h

  Line 1 in b4dede0

  #ifndef DataFormats_SiPixelDigiSoA_interface_alpaka_SiPixelDigiErrorsUtilities_h

  
  Is this file still necessary, given that the contents are commented out?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelDigiSoA/interface/alpaka/SiPixelDigisCollection.h

  Lines 24 to 27 in b4dede0

  	template <>
  	struct CopyToHost<ALPAKA_ACCELERATOR_NAMESPACE::SiPixelDigisCollection> {
  	template <typename TQueue>
  	static auto copyAsync(TQueue &queue, ALPAKA_ACCELERATOR_NAMESPACE::SiPixelDigisCollection const &srcData) {

  • Change with:

     template <typename TDevice>
     struct CopyToHost<SiPixelDigisDevice<TDevice>> {
       template <typename TQueue>
       static auto copyAsync(TQueue &queue, SiPixelDigisDevice<TDevice> const &srcData) {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelDigiSoA/src/classes_def.xml

  Lines 24 to 34 in b4dede0

  	<read
  	sourceClass="PortableHostCollection<SiPixelDigiErrorsSoA>"
  	targetClass="PortableHostCollection<SiPixelDigiErrorsSoA>"
  	version="[1-]"
  	source="SiPixelDigiErrorsSoA layout_;"
  	target="buffer_"
  	embed="false">
  	<![CDATA[
  	PortableHostCollection<SiPixelDigiErrorsSoA>::ROOTReadStreamer(newObj, onfile.layout_);
  	]]>
  	</read>

  
  Replace with

   SET_PORTABLEHOSTCOLLECTION_READ_RULES(PortableHostCollection<SiPixelDigiErrorsSoA>);
  

  in classes.cc?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelDigiSoA/test/BuildFile.xml

  Line 4 in b4dede0

  <flags ALPAKA_BACKENDS="1"/>

  • Change with:

     <flags ALPAKA_BACKENDS="1"/>

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelDigiSoA/test/BuildFile.xml

  Line 10 in b4dede0

  <flags ALPAKA_BACKENDS="1"/>

  • Change with:

     <flags ALPAKA_BACKENDS="1"/>

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelDigiSoA/BuildFile.xml

  Line 1 in b4dede0

  <use name="alpaka"/>

  
  Is the new package really needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/DataFormats/SiPixelRawData/src/classes_def.xml

  Line 17 in b4dede0

  <class name="std::pair<const unsigned int,std::vector<SiPixelRawDataError> >"/>

  
  Why is this dictionary needed?

---

05 January 2024

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/plugins/alpaka/PixelRecHits.h

  Line 91 in b64e14b

  if (threadIdxLocal == 0)

  • Change with:
    Could be

             if (cms::alpakatools::once_per_block(acc))

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/plugins/alpaka/PixelRecHits.h

  Line 62 in b64e14b

  if (0 == threadIdxLocal) {

  • Change with:
    Could be now

             if (cms::alpakatools::once_per_block(acc)) {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/plugins/alpaka/PixelRecHits.h

  Line 27 in b64e14b

  class getHits {

  • Change with:

       class GetHits {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/plugins/alpaka/PixelCPEFastParamsESProducerAlpaka.cc

  Line 56 in b64e14b

  auto cc = setWhatProduced(this, myname);

  
  Strictly speaking the ComponentName parameter is redundant, because the framework already provides appendToDataLabel that would do all this for you (but ok, the pattern exists already).

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/plugins/alpaka/PixelCPEFastParamsESProducerAlpaka.cc

  Lines 71 to 72 in b64e14b

  	const SiPixelLorentzAngle* lorentzAngleWidthProduct = nullptr;
  	lorentzAngleWidthProduct = &iRecord.get(lorentzAngleWidthToken_);

  • Change with:
    How about

       const SiPixelLorentzAngle* lorentzAngleWidthProduct = &iRecord.get(lorentzAngleWidthToken_);

  ?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/plugins/SiPixelRecHitFromSoAAlpaka.cc

  Lines 127 to 128 in b64e14b

  	edm::LogWarning("SiPixelRecHitFromSoAAlpaka") << fmt::sprintf(
  	"Too many clusters %d in module %d. Only the first %d hits will be converted", nhits, gind, maxHitsInModule);

  • Change with:
    How about

         edm::LogWarning("SiPixelRecHitFromSoAAlpaka").format(
             "Too many clusters {} in module {}. Only the first {} hits will be converted", nhits, gind, maxHitsInModule);

  ? Then the fmt/printf.h include. could be removed.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/plugins/BuildFile.xml

  Lines 6 to 7 in b64e14b

  	<use name="HeterogeneousCore/AlpakaCore"/>
  	<use name="HeterogeneousCore/AlpakaInterface"/>

  
  I'd expect AlpakaCore (and maybe AlpakaInterface) dependence to apply only to RecoLocalTrackerSiPixelRecHitsPluginsPortable.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/plugins/BuildFile.xml

  Line 12 in b64e14b

  <use name="cuda"/>

  
  I'd think the cuda dependence here would get applied also to RecoLocalTrackerSiPixelRecHitsPluginsPortable, so it should be moved under the RecoLocalTrackerSiPixelRecHitsPlugins definition too (inside an iftool element).

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/interface/pixelCPEforDevice.h

  Line 17 in b64e14b

  // Nesting this namespace to prevent conflicts with pixelCPEForDevice.h

  
  What does the "Nesting" refer to?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/interface/alpaka/PixelCPEFastParamsCollection.h

  Line 37 in b64e14b

  PixelCPEFastParamsDevice<TDevice, TrackerTraits> dstData(queue); //srcData->metadata().size(), queue);

  
  Can the commented code be removed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/interface/alpaka/PixelCPEFastParamsCollection.h

  Lines 16 to 24 in b64e14b

  	#ifdef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED
  	template <typename TrackerTraits>
  	using PixelCPEFastParamsCollection = PixelCPEFastParamsHost<TrackerTraits>;
  	#else
  	template <typename TrackerTraits>
  	using PixelCPEFastParamsCollection = PixelCPEFastParamsDevice<Device, TrackerTraits>;
  	#endif
  	template <typename TrackerTraits>
  	using PixelCPEFastParams = PixelCPEFastParamsCollection<TrackerTraits>;

  • Change with:
    How about

     template <typename TrackerTraits>
     using PixelCPEFastParams = std::conditional_v<std::is_same_v<Device, alpaka::DevCpu>, PixelCPEFastParamsHost<TrackerTraits>, PixelCPEFastParamsDevice<Device, TrackerTraits>>;

  ?

  Does the PixelCPEFastParamsCollection alias template provide any value in addition to PixelCPEFastParams?

  These definitions would be simpler if the PortableObject<T> could be used for both the host and device. Also the CopyToDevice specialization would not be needed. The code looks though such a change would require some re-engineering, that is perhaps better to leave for later time.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/interface/PixelCPEFastParamsHost.h

  Line 29 in b64e14b

  PixelCPEFastParamsHost(edm::ParameterSet const& conf,

  
  EventSetup data products should not depend on edm::ParameterSet (or ParameterSetDescription). Dealing with ParameterSet should be done only in the ESProducer.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/interface/PixelCPEFastParamsDevice.h

  Line 13 in b64e14b

  class PixelCPEFastParamsDevice {

  • Change with:
    Could this class, in principle, be replaced with

   using PixelCPEFastParamsDevice = PortableDeviceObject<pixelCPEforDevice::ParamsOnDeviceT<TrackerTraits>, TDev>;

  ? (another question is then if the alias template would make sense)

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/interface/PixelCPEFastParamsDevice.h

  Line 10 in b64e14b

  #include "pixelCPEforDevice.h"

  
  #includes in interface should use the full path

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelRecHits/interface/PixelCPEFastParamsDevice.h

  Lines 5 to 8 in b64e14b

  	#include "HeterogeneousCore/AlpakaInterface/interface/config.h"
  	#include "HeterogeneousCore/AlpakaInterface/interface/memory.h"
  	#include "FWCore/ParameterSet/interface/ParameterSet.h"
  	#include "RecoLocalTracker/SiPixelRecHits/interface/PixelCPEGenericBase.h"

  • Change with:
    Of these #includes only the memory.h seems to be needed, or did I miss something?

   #include "HeterogeneousCore/AlpakaInterface/interface/memory.h"

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.h

  Line 125 in b64e14b

  class WordFedAppender {

  
  WordFedAppender seems to be independent of TrackerTraits, so it could be moved outside of the SiPixelRawToClusterKernel (that should lead to a little bit less code to be generated).

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.h

  Line 33 in b64e14b

  const uint32_t layerStartBit = 20;

  
  How about making all of these constexpr?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Line 782 in b64e14b

  const_cast<uint32_t const *>(clusters_d->view().clusModuleStart() + numberOfModules),

  • Change with:

             clusters_d->const_view().clusModuleStart() + numberOfModules,

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Line 747 in b64e14b

  128; /// should be larger than maxPixInModule/16 aka (maxPixInModule/maxiter in the kernel)

  
  Maybe move the comment to preceding line for better formatting?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Line 689 in b64e14b

  const_cast<uint32_t const *>(clusters_d->view().clusModuleStart() + numberOfModules),

  • Change with:
    I'd suggest to remove const_cast as not really necessary (given it is a red flag in most cases). The constness of the pointed-to memory region could be achieved with

               clusters_d->const_view().clusModuleStart() + numberOfModules,

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Lines 649 to 650 in b64e14b

  	const auto threadsPerBlockFindClus =
  	512; //((TrackerTraits::maxPixInModule / 16 + 128 - 1) / 128) * 128; /// should be larger than maxPixInModule/16 aka (maxPixInModule/maxiter in the kernel)

  
  Is the commented out formula useful? Maybe move the comments on preceding line(s) for better formatting?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Line 576 in b64e14b

  if constexpr (std::is_same_v<Device, alpaka_common::DevHost>) {

  • Change with:
    Should (or could) this be

             if constexpr (cms::alpakatools::requires_single_thread_per_block_v<Acc1D>) {

  ?

  (in principle the lambda call could be replaced with a ternary)

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Line 561 in b64e14b

  Queue &queue) {

  
  Maybe move the Queue& as the first argument, to be more consistent with Alpaka's interfaces?

  (also elsewhere)

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Line 586 in b64e14b

  ALPAKA_ASSERT_OFFLOAD(0 == wordCounter % 2);

  • Change with:
    Since this is host code, maybe this should be

           assert(0 == wordCounter % 2);

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Lines 573 to 574 in b64e14b

  	if (wordCounter) // protect in case of empty event....
  	{

  • Change with:
    How about

           // protect in case of empty event....
         if (wordCounter) {

  ?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Line 462 in b64e14b

  struct fillHitsModuleStart {

  • Change with:

       struct FillHitsModuleStart {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToClusterKernel.dev.cc

  Line 138 in b64e14b

  case (1): {

  • Change with:
    Maybe drop the (unnecessary) parentheses

           case 1: {

  ?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelRawToCluster.cc

  Line 152 in b64e14b

  auto Gains = SiPixelGainCalibrationForHLTDevice(1, iEvent.queue());

  • Change with:

       auto gains = SiPixelGainCalibrationForHLTDevice(1, iEvent.queue());

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/CalibPixel.h

  Line 21 in b64e14b

  #include "RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelClusterThresholds.h"

  
  Strictly speaking this #include is not allowed. I'd suggest to move the SiPixelClusterThresholds.h to RecoLocalTracker/SiPixelClusterizer/interface, and maybe add a comment that it is an implementation detail of the package.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelPhase2DigiToCluster.cc

  Line 117 in b64e14b

  for (auto DSViter = input.begin(); DSViter != input.end(); DSViter++) {

  
  How about range-for?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelPhase2DigiToCluster.cc

  Line 110 in b64e14b

  SiPixelDigisHost digis_h(nDigis, iEvent.queue());

  
  Maybe move this line below the nDigis_ == 0 check to avoid constructing it when not needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/SiPixelPhase2DigiToCluster.cc

  Lines 107 to 109 in b64e14b

  	for (auto DSViter = input.begin(); DSViter != input.end(); DSViter++) {
  	nDigis = nDigis + DSViter->size();
  	}

  • Change with:
    How about

       for (auto const& ds : input) {
         nDigis = nDigis + ds.size();
       }

  ?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Lines 270 to 275 in b64e14b

  	#ifndef ALPAKA_ACC_CPU_B_SEQ_T_SEQ_ENABLED
  	constexpr uint32_t threadDimension = 1;
  	#else
  	// NB: can be tuned.
  	constexpr uint32_t threadDimension = 256;
  	#endif

  • Change with:
    How about

             // NB: can be tuned.
             constexpr uint32_t threadDimension = cms::alpakatools::requires_single_thread_per_block_v<TAcc> ? 1 : 256;

  ?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Line 210 in b64e14b

  if constexpr (not isPhase2 and false) { //FIXME remove THIS

  
  Does this FIXME still need to be addressed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Line 122 in b64e14b

  struct findClus {

  • Change with:

       struct FindClus {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Line 98 in b64e14b

  if (0 == threadIdxGlobal)

  • Change with:
    Could now be

           if (cms::alpakatools::once_per_grid(acc))

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Line 87 in b64e14b

  struct countModules {

  • Change with:

       struct CountModules {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Line 69 in b64e14b

  uint32_t old_word = status[index];

  
  This is more of a note for longer term future. When the threads of a block execute in lock step, this is probably fine. But if/when they are not in lockstep, in principle this line has a data race with the alpaka::atomicCas() on line 80.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Line 40 in b64e14b

  ALPAKA_FN_ACC ALPAKA_FN_INLINE constexpr static uint32_t getIndex(uint16_t x, uint16_t y) {

  • Change with:
    Since these functions are defined in a namespace (as opposed to a class), and in a header, they should not be static

         ALPAKA_FN_ACC ALPAKA_FN_INLINE constexpr uint32_t getIndex(uint16_t x, uint16_t y) {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Line 33 in b64e14b

  enum Status : uint32_t { kEmpty = 0x00, kFound = 0x01, kDuplicate = 0x03 };

  
  Given that the Status seems to be always initialized as a result of bit operations, and also the integer value used in bit operations, I'm wondering if defining it as an enum gives any value compared to defining these as constexpr uint32_t constants?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/PixelClustering.h

  Line 27 in b64e14b

  namespace PixelStatus {

  • Change with:

       namespace pixelStatus {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/ClusterChargeCut.h

  Line 92 in b64e14b

  if (threadIdxLocal == 0)

  • Change with:
    Could now be

             if (cms::alpakatools::once_per_block(acc))

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/CalibPixel.h

  Line 20 in b64e14b

  #include "HeterogeneousCore/AlpakaInterface/interface/traits.h"

  • Suggestion: This header seems to not to be needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/CalibPixel.h

  Line 19 in b64e14b

  #include "HeterogeneousCore/AlpakaInterface/interface/config.h"

  • Suggestion: Since the code is not in ALPAKA_ACCLERATOR_NAMESPACE, maybe this #include is not needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/ClusterChargeCut.h

  Line 10 in b64e14b

  #include "HeterogeneousCore/AlpakaInterface/interface/config.h"

  • Suggestion: Since the code is not in ALPAKA_ACCELERATOR_NAMESPACE, maybe this #include is not needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/ClusterChargeCut.h

  Line 15 in b64e14b

  // namespace ALPAKA_ACCELERATOR_NAMESPACE {

  • Suggestion: Remove?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/ClusterChargeCut.h

  Line 19 in b64e14b

  struct clusterChargeCut {

  • Change with:

     struct ClusterChargeCut {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/CalibPixel.h

  Line 102 in b64e14b

  if (0 == threadIdxGlobal)

  • Change with:
    Could now be

         if (cms::alpakatools::once_per_grid(acc))

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/CalibPixel.h

  Line 88 in b64e14b

  struct calibDigisPhase2 {

  • Change with:

     struct CalibDigisPhase2 {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/CalibPixel.h

  Line 71 in b64e14b

  if (threadIdxGlobal == 0)

  • Change with:
    Could now be

               if (cms::alpakatools::one_per_grid(acc))

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/CalibPixel.h

  Line 43 in b64e14b

  if (threadIdxGlobal == 0) {

  • Change with:
    Could now be

         if (cms::alpakatools::once_per_grid(acc)) {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/alpaka/CalibPixel.h

  Line 29 in b64e14b

  struct calibDigis {

  • Change with:

     struct CalibDigis {

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelDigisClustersFromSoAAlpaka.cc

  Line 52 in b64e14b

  storeDigis_(iConfig.getParameter<bool>("produceDigis") && iConfig.getParameter<bool>("storeDigis")),

  • Change with:
    Could be

         storeDigis_(produceDigis_ && iConfig.getParameter<bool>("storeDigis")),

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/SiPixelDigisClustersFromSoAAlpaka.cc

  Line 3 in b64e14b

  #include "DataFormats/Common/interface/Handle.h"

  • Suggestion: Doesn't seem to be needed

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/SiPixelClusterizer/plugins/BuildFile.xml

  Lines 15 to 17 in b64e14b

  	<use name="HeterogeneousCore/CUDACore"/>
  	<use name="CUDADataFormats/SiPixelCluster"/>
  	<use name="CUDADataFormats/SiPixelDigi"/>

  
  Strictly speaking moving these dependencies under the iftool is incorrect, because there are .cc files that depend on these packages. On the other hand, the non-CUDA build is broken no matter what, so the practical impact is negligible.

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/RecoLocalTracker/Records/BuildFile.xml

  Line 11 in b64e14b

  <flags ALPAKA_BACKENDS="1"/>

  • Suggestion: This change does not seem to be needed?

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/EventFilter/SiPixelRawToDigi/plugins/SiPixelDigiErrorsFromSoAAlpaka.cc

  Line 98 in b64e14b

  // assert(formatterErrors != nullptr); // TODO: check what is happening here

  • Suggestion: This line can probably be removed

• Pixel Alpaka Migration: Local Reconstruction [III] cms-sw/cmssw#41285 (comment) by @makortel
  

  cmssw/EventFilter/SiPixelRawToDigi/plugins/SiPixelDigiErrorsFromSoAAlpaka.cc

  Line 14 in b64e14b

  #include "FWCore/Framework/interface/ESTransientHandle.h"

  • Suggestion: Doesn't seem to be needed

[fwyzard]

Last round of comments and follow ups:

• from Matti:

  Is there are general agreement then that the new SoA based data format classes are better placed in new packages rather than adding the new classes in existing packages? I.e.

  • DataFormats/SiPixelClusterSoA vs. DataFormats/SiPixelCluster
  • DataFormats/SiPixelDigiSoA vs. DataFormats/SiPixelDigi
  • DataFormats/TrackingRecHitSoA vs DataFormats/TrackingRecHit

• Address the TODO in DataFormats/SiPixelClusterSoA/interface/ClusteringConstants.h
  See this discussion.

• Address why we need using namespace pixelTopology; in DataFormats/TrackingRecHitSoA/src/classes.h and DataFormats/TrackingRecHitSoA/src/alpaka/classes_*.h.

• Move the queue parameter to the beginning of the parameter list, and pass it by reference if possible.
  See here, here and here.

• Optimise the copies and synchronisations in DataFormats/TrackingRecHitSoA/interface/TrackingRecHitsDevice.h
  See here and here.

• Find a better way to create CPU-only module configurations.
  See also #43780.