Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patatrack integration - Hcal conditions (13/N) #32039

Merged
merged 2 commits into from Dec 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions CondFormats/DataRecord/interface/HcalCombinedRecordsGPU.h
@@ -0,0 +1,19 @@
#ifndef CondFormats_DataRecord_interface_HcalCombinedRecordsGPU_h
#define CondFormats_DataRecord_interface_HcalCombinedRecordsGPU_h

#include "CondFormats/DataRecord/interface/HcalPedestalWidthsRcd.h"
#include "CondFormats/DataRecord/interface/HcalPedestalsRcd.h"
#include "CondFormats/DataRecord/interface/HcalQIEDataRcd.h"
#include "CondFormats/DataRecord/interface/HcalQIETypesRcd.h"
#include "FWCore/Framework/interface/DependentRecordImplementation.h"

template <typename... Sources>
class HcalCombinedRecord : public edm::eventsetup::DependentRecordImplementation<HcalCombinedRecord<Sources...>,
edm::mpl::Vector<Sources...>> {};

using HcalConvertedPedestalsRcd = HcalCombinedRecord<HcalPedestalsRcd, HcalQIEDataRcd, HcalQIETypesRcd>;

using HcalConvertedPedestalWidthsRcd =
HcalCombinedRecord<HcalPedestalsRcd, HcalPedestalWidthsRcd, HcalQIEDataRcd, HcalQIETypesRcd>;

#endif // CondFormats_DataRecord_interface_HcalCombinedRecordsGPU_h
5 changes: 5 additions & 0 deletions CondFormats/DataRecord/src/HcalCombinedRecordsGPU.cc
@@ -0,0 +1,5 @@
#include "CondFormats/DataRecord/interface/HcalCombinedRecordsGPU.h"
#include "FWCore/Framework/interface/eventsetuprecord_registration_macro.h"

EVENTSETUP_RECORD_REG(HcalConvertedPedestalsRcd);
EVENTSETUP_RECORD_REG(HcalConvertedPedestalWidthsRcd);
10 changes: 6 additions & 4 deletions CondFormats/HcalObjects/BuildFile.xml
@@ -1,13 +1,15 @@
<ifrelease name="_UBSAN_">
<flags REM_CXXFLAGS="-fno-omit-frame-pointer -fsanitize=undefined"/>
</ifrelease>
<use name="boost_serialization"/>
<use name="CondFormats/Serialization"/>
<use name="DataFormats/DetId"/>
<use name="DataFormats/HcalDetId"/>
<use name="Geometry/CaloTopology"/>
<use name="FWCore/Utilities"/>
<use name="FWCore/MessageLogger"/>
<use name="CondFormats/Serialization"/>
<use name="boost_serialization"/>
<use name="FWCore/Utilities"/>
<use name="Geometry/CaloTopology"/>
<use name="HeterogeneousCore/CUDACore"/>
<use name="HeterogeneousCore/CUDAUtilities"/>
<export>
<lib name="1"/>
</export>
@@ -0,0 +1,12 @@
#ifndef CondFormats_HcalObjects_interface_HcalConvertedEffectivePedestalWidthsGPU_h
#define CondFormats_HcalObjects_interface_HcalConvertedEffectivePedestalWidthsGPU_h

#include "CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h"

// similar to converted effective pedestals
class HcalConvertedEffectivePedestalWidthsGPU final : public HcalConvertedPedestalWidthsGPU {
public:
using HcalConvertedPedestalWidthsGPU::HcalConvertedPedestalWidthsGPU;
};

#endif // RecoLocalCalo_HcalRecAlgos_interface_HcalConvertedEffectivePedestalWidthsGPU_h
@@ -0,0 +1,14 @@
#ifndef CondFormats_HcalObjects_interface_HcalConvertedEffectivePedestalsGPU_h
#define CondFormats_HcalObjects_interface_HcalConvertedEffectivePedestalsGPU_h

#include "CondFormats/HcalObjects/interface/HcalConvertedPedestalsGPU.h"

// Separate access to effective and regular pedestals
// No need to transfer/rearrange effective or vice versa if they are not going
// to be used
class HcalConvertedEffectivePedestalsGPU final : public HcalConvertedPedestalsGPU {
public:
using HcalConvertedPedestalsGPU::HcalConvertedPedestalsGPU;
};

#endif // RecoLocalCalo_HcalRecAlgos_interface_HcalConvertedEffectivePedestalsGPU_h
43 changes: 43 additions & 0 deletions CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h
@@ -0,0 +1,43 @@
#ifndef CondFormats_HcalObjects_interface_HcalConvertedPedestalWidthsGPU_h
#define CondFormats_HcalObjects_interface_HcalConvertedPedestalWidthsGPU_h

#include "CondFormats/HcalObjects/interface/HcalPedestalWidths.h"
#include "CondFormats/HcalObjects/interface/HcalPedestals.h"
#include "CondFormats/HcalObjects/interface/HcalQIEData.h"
#include "CondFormats/HcalObjects/interface/HcalQIETypes.h"
#include "FWCore/Utilities/interface/propagate_const_array.h"
#include "HeterogeneousCore/CUDAUtilities/interface/device_unique_ptr.h"

#ifndef __CUDACC__
#include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
#include "HeterogeneousCore/CUDACore/interface/ESProduct.h"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a dependence on CUDA acceptable for CondFormats? Or would we need e.g. CUDACondFormats for these? (these are not supposed to be stored in the CondDB in any case)

Just to note that in the next revision of the CUDA framework support I hope to remove the explicit use of cms::cuda::ESProduct as a member of the "payload", and instead wrap the payload similarly to cms::cuda::Product<T> (for EDProducts) for th ES product.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note that in the next revision of the CUDA framework support I hope to remove the explicit use of cms::cuda::ESProduct as a member of the "payload"

That would remove the dependency on CUDA from this package ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note that in the next revision of the CUDA framework support I hope to remove the explicit use of cms::cuda::ESProduct as a member of the "payload"

That would remove the dependency on CUDA from this package ?

Very likely yes. Essentially e.g. HcalConvertedPedestalWidthsGPU would change such that (e.g.)

  • HcalConvertedPedestalWidthsGPU::Product becomes HcalConvertedPedestalWidthsGPU
  • The logic from HcalConvertedPedestalWidthsGPU::getProduct() moves into the ESProducer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, then I would leave this part as is for the time being.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note that in the next revision of the CUDA framework support I hope to remove the explicit use of cms::cuda::ESProduct as a member of the "payload"

That would remove the dependency on CUDA from this package ?

Very likely yes.

On a further thought I have to take it back. If the payload uses e.g. cms::cuda::device::unique_ptr (which I hope the CUDA ESProducts would move to at that point), the dependence on HeterogeneousCore/CUDAUtilities would be unavoidable.

#endif

class HcalConvertedPedestalWidthsGPU {
public:
struct Product {
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> values;
};

#ifndef __CUDACC__
// order matters!
HcalConvertedPedestalWidthsGPU(HcalPedestals const&,
HcalPedestalWidths const&,
HcalQIEData const&,
HcalQIETypes const&);

// will trigger deallocation of Product thru ~Product
~HcalConvertedPedestalWidthsGPU() = default;

// get device pointers
Product const& getProduct(cudaStream_t) const;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned by this function (and many below) effectively returning "const pointer to non-const" instead of "(const) pointers to const". Then it would be very easy (as in compiler would not catch) to have code that accidentally modifies the pointed-to data (which is not allowed for ES products).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think that changing the definition of Product to

  struct Product {
    ~Product();
    edm::propagate_const<float*> values;
  };

would fix this potential issue ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe edm::propagate_const would solve issue. I suppose it would require declaring ~all methods of propagate_const as constexpr for it to work in the device code.

Copy link
Contributor

@fwyzard fwyzard Nov 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually these changes compile fine:

diff --git a/CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h b/CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h
index e3adb59ca54e..1afad7ca3a66 100644
--- a/CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h
+++ b/CondFormats/HcalObjects/interface/HcalConvertedPedestalWidthsGPU.h
@@ -5,6 +5,7 @@
 #include "CondFormats/HcalObjects/interface/HcalPedestalWidths.h"
 #include "CondFormats/HcalObjects/interface/HcalQIEData.h"
 #include "CondFormats/HcalObjects/interface/HcalQIETypes.h"
+#include "FWCore/Utilities/interface/propagate_const.h"
 
 #ifndef __CUDACC__
 #include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
@@ -15,7 +16,7 @@ class HcalConvertedPedestalWidthsGPU {
 public:
   struct Product {
     ~Product();
-    float* values;
+    edm::propagate_const<float*> values;
   };
 
 #ifndef __CUDACC__
diff --git a/CondFormats/HcalObjects/src/HcalConvertedPedestalWidthsGPU.cc b/CondFormats/HcalObjects/src/HcalConvertedPedestalWidthsGPU.cc
index 1d7ca5b3394c..0af2b5eaade2 100644
--- a/CondFormats/HcalObjects/src/HcalConvertedPedestalWidthsGPU.cc
+++ b/CondFormats/HcalObjects/src/HcalConvertedPedestalWidthsGPU.cc
@@ -145,6 +145,8 @@ HcalConvertedPedestalWidthsGPU::Product const& HcalConvertedPedestalWidthsGPU::g
   auto const& product = product_.dataForCurrentDeviceAsync(
       cudaStream, [this](HcalConvertedPedestalWidthsGPU::Product& product, cudaStream_t cudaStream) {
         // malloc
-        cudaCheck(cudaMalloc((void**)&product.values, this->values_.size() * sizeof(float)));
+        float* values;
+        cudaCheck(cudaMalloc(&values, values_.size() * sizeof(float)));
+        product.values = values;
 
         // transfer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually these changes compile fine:

Ah, right, probably HcalConvertedPedestalWidthsGPU::Product is not passed to device code as such, but the individual arrays themselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@makortel can you confirm that the implementation that uses edm::propagate_const_array is now fine ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you confirm that the implementation that uses edm::propagate_const_array is now fine ?

Visually the use of edm::propagate_const_array looks good now, thanks.


private:
uint64_t totalChannels_;
std::vector<float, cms::cuda::HostAllocator<float>> values_;

cms::cuda::ESProduct<Product> product_;
#endif
};

#endif
42 changes: 42 additions & 0 deletions CondFormats/HcalObjects/interface/HcalConvertedPedestalsGPU.h
@@ -0,0 +1,42 @@
#ifndef CondFormats_HcalObjects_interface_HcalConvertedPedestalsGPU_h
#define CondFormats_HcalObjects_interface_HcalConvertedPedestalsGPU_h

#include "CondFormats/HcalObjects/interface/HcalPedestals.h"
#include "CondFormats/HcalObjects/interface/HcalQIEData.h"
#include "CondFormats/HcalObjects/interface/HcalQIETypes.h"
#include "FWCore/Utilities/interface/propagate_const_array.h"
#include "HeterogeneousCore/CUDAUtilities/interface/device_unique_ptr.h"

#ifndef __CUDACC__
#include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
#include "HeterogeneousCore/CUDACore/interface/ESProduct.h"
#endif

class HcalConvertedPedestalsGPU {
public:
struct Product {
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> values;
};

#ifndef __CUDACC__
// order matters!
HcalConvertedPedestalsGPU(HcalPedestals const&, HcalQIEData const&, HcalQIETypes const&);

// will trigger deallocation of Product thru ~Product
~HcalConvertedPedestalsGPU() = default;

// get device pointers
Product const& getProduct(cudaStream_t) const;

uint32_t offsetForHashes() const { return offsetForHashes_; }

protected:
uint64_t totalChannels_;
uint32_t offsetForHashes_;
std::vector<float, cms::cuda::HostAllocator<float>> values_;

cms::cuda::ESProduct<Product> product_;
#endif
};

#endif
40 changes: 40 additions & 0 deletions CondFormats/HcalObjects/interface/HcalGainWidthsGPU.h
@@ -0,0 +1,40 @@
#ifndef CondFormats_HcalObjects_interface_HcalGainWidthsGPU_h
#define CondFormats_HcalObjects_interface_HcalGainWidthsGPU_h

#include "CondFormats/HcalObjects/interface/HcalGainWidths.h"
#include "FWCore/Utilities/interface/propagate_const_array.h"
#include "HeterogeneousCore/CUDAUtilities/interface/device_unique_ptr.h"

#ifndef __CUDACC__
#include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
#include "HeterogeneousCore/CUDACore/interface/ESProduct.h"
#endif

class HcalGainWidthsGPU {
public:
struct Product {
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> value0;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> value1;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> value2;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> value3;
};

#ifndef __CUDACC__
// rearrange reco params
HcalGainWidthsGPU(HcalGainWidths const &);

// will trigger deallocation of Product thru ~Product
~HcalGainWidthsGPU() = default;

// get device pointers
Product const &getProduct(cudaStream_t) const;

private:
uint64_t totalChannels_;
std::vector<float, cms::cuda::HostAllocator<float>> value0_, value1_, value2_, value3_;

cms::cuda::ESProduct<Product> product_;
#endif
};

#endif
37 changes: 37 additions & 0 deletions CondFormats/HcalObjects/interface/HcalGainsGPU.h
@@ -0,0 +1,37 @@
#ifndef CondFormats_HcalObjects_interface_HcalGainsGPU_h
#define CondFormats_HcalObjects_interface_HcalGainsGPU_h

#include "CondFormats/HcalObjects/interface/HcalGains.h"
#include "FWCore/Utilities/interface/propagate_const_array.h"
#include "HeterogeneousCore/CUDAUtilities/interface/device_unique_ptr.h"

#ifndef __CUDACC__
#include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
#include "HeterogeneousCore/CUDACore/interface/ESProduct.h"
#endif

class HcalGainsGPU {
public:
struct Product {
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> values;
};

#ifndef __CUDACC__
// rearrange reco params
HcalGainsGPU(HcalGains const&);

// will trigger deallocation of Product thru ~Product
~HcalGainsGPU() = default;

// get device pointers
Product const& getProduct(cudaStream_t) const;

private:
uint64_t totalChannels_;
std::vector<float, cms::cuda::HostAllocator<float>> values_;

cms::cuda::ESProduct<Product> product_;
#endif
};

#endif
36 changes: 36 additions & 0 deletions CondFormats/HcalObjects/interface/HcalLUTCorrsGPU.h
@@ -0,0 +1,36 @@
#ifndef CondFormats_HcalObjects_interface_HcalLUTCorrsGPU_h
#define CondFormats_HcalObjects_interface_HcalLUTCorrsGPU_h

#include "CondFormats/HcalObjects/interface/HcalLUTCorrs.h"
#include "FWCore/Utilities/interface/propagate_const_array.h"
#include "HeterogeneousCore/CUDAUtilities/interface/device_unique_ptr.h"

#ifndef __CUDACC__
#include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
#include "HeterogeneousCore/CUDACore/interface/ESProduct.h"
#endif

class HcalLUTCorrsGPU {
public:
struct Product {
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> value;
};

#ifndef __CUDACC__
// rearrange reco params
HcalLUTCorrsGPU(HcalLUTCorrs const&);

// will trigger deallocation of Product thru ~Product
~HcalLUTCorrsGPU() = default;

// get device pointers
Product const& getProduct(cudaStream_t) const;

private:
std::vector<float, cms::cuda::HostAllocator<float>> value_;

cms::cuda::ESProduct<Product> product_;
#endif
};

#endif
71 changes: 71 additions & 0 deletions CondFormats/HcalObjects/interface/HcalPedestalWidthsGPU.h
@@ -0,0 +1,71 @@
#ifndef CondFormats_HcalObjects_interface_HcalPedestalWidthsGPU_h
#define CondFormats_HcalObjects_interface_HcalPedestalWidthsGPU_h

#include "CondFormats/HcalObjects/interface/HcalPedestalWidths.h"
#include "FWCore/Utilities/interface/propagate_const_array.h"
#include "HeterogeneousCore/CUDAUtilities/interface/device_unique_ptr.h"

#ifndef __CUDACC__
#include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
#include "HeterogeneousCore/CUDACore/interface/ESProduct.h"
#endif

class HcalPedestalWidthsGPU {
public:
struct Product {
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma00;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma01;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma02;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma03;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma10;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma11;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma12;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma13;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma20;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma21;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma22;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma23;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma30;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma31;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma32;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> sigma33;
};

#ifndef __CUDACC__
// rearrange reco params
HcalPedestalWidthsGPU(HcalPedestalWidths const&);

// will trigger deallocation of Product thru ~Product
~HcalPedestalWidthsGPU() = default;

// get device pointers
Product const& getProduct(cudaStream_t) const;

// as in cpu version
bool unitIsADC() const { return unitIsADC_; }

private:
bool unitIsADC_;
uint64_t totalChannels_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma00_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma01_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma02_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma03_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma10_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma11_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma12_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma13_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma20_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma21_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma22_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma23_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma30_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma31_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma32_;
std::vector<float, cms::cuda::HostAllocator<float>> sigma33_;

cms::cuda::ESProduct<Product> product_;
#endif
};

#endif
46 changes: 46 additions & 0 deletions CondFormats/HcalObjects/interface/HcalPedestalsGPU.h
@@ -0,0 +1,46 @@
#ifndef CondFormats_HcalObjects_interface_HcalPedestalsGPU_h
#define CondFormats_HcalObjects_interface_HcalPedestalsGPU_h

#include "CondFormats/HcalObjects/interface/HcalPedestals.h"
#include "FWCore/Utilities/interface/propagate_const_array.h"
#include "HeterogeneousCore/CUDAUtilities/interface/device_unique_ptr.h"

#ifndef __CUDACC__
#include "HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h"
#include "HeterogeneousCore/CUDACore/interface/ESProduct.h"
#endif

class HcalPedestalsGPU {
public:
struct Product {
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> values;
edm::propagate_const_array<cms::cuda::device::unique_ptr<float[]>> widths;
};

#ifndef __CUDACC__
// rearrange reco params
HcalPedestalsGPU(HcalPedestals const &);

// will trigger deallocation of Product thru ~Product
~HcalPedestalsGPU() = default;

// get device pointers
Product const &getProduct(cudaStream_t) const;

// as in cpu version
bool unitIsADC() const { return unitIsADC_; }

uint32_t offsetForHashes() const { return offsetForHashes_; }

private:
bool unitIsADC_;
uint64_t totalChannels_;
uint32_t offsetForHashes_;
std::vector<float, cms::cuda::HostAllocator<float>> values_;
std::vector<float, cms::cuda::HostAllocator<float>> widths_;

cms::cuda::ESProduct<Product> product_;
#endif
};

#endif