From 800fd9c8d88abc2a79fe81b8b00b8f3ecf9c254c Mon Sep 17 00:00:00 2001 From: zhangbo9674 Date: Fri, 9 Jun 2023 09:41:01 +0000 Subject: [PATCH 1/6] fix ir storage bug --- paddle/ir/core/storage_manager.cc | 20 ++++++++++++++------ paddle/ir/core/storage_manager.h | 23 +++++++++++++++++++++-- 2 files changed, 35 insertions(+), 8 deletions(-) diff --git a/paddle/ir/core/storage_manager.cc b/paddle/ir/core/storage_manager.cc index 8bc74b23c1d10a..7ed63f3cfb7a54 100644 --- a/paddle/ir/core/storage_manager.cc +++ b/paddle/ir/core/storage_manager.cc @@ -25,11 +25,17 @@ namespace ir { struct ParametricStorageManager { using StorageBase = StorageManager::StorageBase; - ParametricStorageManager() {} + explicit ParametricStorageManager( + std::function destructor) + : destructor_(destructor) {} ~ParametricStorageManager() { for (const auto &instance : parametric_instances_) { - delete instance.second; + if (destructor_) { + destructor_(instance.second); + } else { + delete instance.second; + } } parametric_instances_.clear(); } @@ -37,7 +43,7 @@ struct ParametricStorageManager { // Get the storage of parametric type, if not in the cache, create and // insert the cache. StorageBase *GetOrCreate(std::size_t hash_value, - std::function equal_func, + std::function equal_func, std::function constructor) { if (parametric_instances_.count(hash_value) != 0) { auto pr = parametric_instances_.equal_range(hash_value); @@ -62,6 +68,7 @@ struct ParametricStorageManager { // In order to prevent hash conflicts, the unordered_multimap data structure // is used for storage. std::unordered_multimap parametric_instances_; + std::function destructor_; }; StorageManager::StorageManager() {} @@ -95,12 +102,13 @@ StorageManager::StorageBase *StorageManager::GetParameterlessStorageImpl( return parameterless_instance; } -void StorageManager::RegisterParametricStorageImpl(TypeId type_id) { +void StorageManager::RegisterParametricStorageImpl( + TypeId type_id, std::function destructor) { std::lock_guard guard(parametric_instance_lock_); VLOG(4) << "Register a parametric storage of: [TypeId_hash=" << std::hash()(type_id) << "]."; - parametric_instance_.emplace(type_id, - std::make_unique()); + parametric_instance_.emplace( + type_id, std::make_unique(destructor)); } void StorageManager::RegisterParameterlessStorageImpl( diff --git a/paddle/ir/core/storage_manager.h b/paddle/ir/core/storage_manager.h index 6b20afb8a8067c..678f497dbccbb4 100644 --- a/paddle/ir/core/storage_manager.h +++ b/paddle/ir/core/storage_manager.h @@ -98,9 +98,27 @@ class StorageManager { /// /// \param type_id The type id of the AbstractType. /// + template + struct IsTriviallyDestructible { + static bool call() { return false; } + }; + + template + struct IsTriviallyDestructible< + Storage, + typename std::enable_if< + std::is_trivially_destructible::value>::type> { + static bool call() { return true; } + }; + template void RegisterParametricStorage(TypeId type_id) { - return RegisterParametricStorageImpl(type_id); + if (IsTriviallyDestructible::call()) { + return RegisterParametricStorageImpl(type_id, nullptr); + } + return RegisterParametricStorageImpl(type_id, [](StorageBase *storage) { + static_cast(storage)->~Storage(); + }); } /// @@ -129,7 +147,8 @@ class StorageManager { StorageBase *GetParameterlessStorageImpl(TypeId type_id); - void RegisterParametricStorageImpl(TypeId type_id); + void RegisterParametricStorageImpl( + TypeId type_id, std::function destructor); void RegisterParameterlessStorageImpl( TypeId type_id, std::function constructor); From 04d687113e04833c9b40ac3f847c56696c12bea9 Mon Sep 17 00:00:00 2001 From: zhangbo9674 Date: Fri, 9 Jun 2023 11:37:26 +0000 Subject: [PATCH 2/6] refine code --- paddle/ir/core/storage_manager.cc | 9 ++++----- paddle/ir/core/storage_manager.h | 5 +++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/paddle/ir/core/storage_manager.cc b/paddle/ir/core/storage_manager.cc index 7ed63f3cfb7a54..93018aab36c7dd 100644 --- a/paddle/ir/core/storage_manager.cc +++ b/paddle/ir/core/storage_manager.cc @@ -30,12 +30,11 @@ struct ParametricStorageManager { : destructor_(destructor) {} ~ParametricStorageManager() { + IR_ENFORCE( + destructor_, + "The desturctor of ParametricStorageManager should not be nullptr."); for (const auto &instance : parametric_instances_) { - if (destructor_) { - destructor_(instance.second); - } else { - delete instance.second; - } + destructor_(instance.second); } parametric_instances_.clear(); } diff --git a/paddle/ir/core/storage_manager.h b/paddle/ir/core/storage_manager.h index 678f497dbccbb4..b52d538e7dd26e 100644 --- a/paddle/ir/core/storage_manager.h +++ b/paddle/ir/core/storage_manager.h @@ -114,10 +114,11 @@ class StorageManager { template void RegisterParametricStorage(TypeId type_id) { if (IsTriviallyDestructible::call()) { - return RegisterParametricStorageImpl(type_id, nullptr); + return RegisterParametricStorageImpl( + type_id, [](StorageBase *storage) { delete storage; }); } return RegisterParametricStorageImpl(type_id, [](StorageBase *storage) { - static_cast(storage)->~Storage(); + delete static_cast(storage); }); } From 83aa36c6e7fed5c99a91a6dfa4d9cdc202e2d823 Mon Sep 17 00:00:00 2001 From: zhangbo9674 Date: Fri, 9 Jun 2023 11:47:12 +0000 Subject: [PATCH 3/6] refine code --- paddle/ir/core/storage_manager.cc | 15 +++++++-------- paddle/ir/core/storage_manager.h | 19 +------------------ 2 files changed, 8 insertions(+), 26 deletions(-) diff --git a/paddle/ir/core/storage_manager.cc b/paddle/ir/core/storage_manager.cc index 93018aab36c7dd..24113ddc2725ad 100644 --- a/paddle/ir/core/storage_manager.cc +++ b/paddle/ir/core/storage_manager.cc @@ -25,16 +25,15 @@ namespace ir { struct ParametricStorageManager { using StorageBase = StorageManager::StorageBase; - explicit ParametricStorageManager( - std::function destructor) - : destructor_(destructor) {} + explicit ParametricStorageManager(std::function destroy) + : destroy_(destroy) {} ~ParametricStorageManager() { IR_ENFORCE( - destructor_, + destroy_, "The desturctor of ParametricStorageManager should not be nullptr."); for (const auto &instance : parametric_instances_) { - destructor_(instance.second); + destroy_(instance.second); } parametric_instances_.clear(); } @@ -67,7 +66,7 @@ struct ParametricStorageManager { // In order to prevent hash conflicts, the unordered_multimap data structure // is used for storage. std::unordered_multimap parametric_instances_; - std::function destructor_; + std::function destroy_; }; StorageManager::StorageManager() {} @@ -102,12 +101,12 @@ StorageManager::StorageBase *StorageManager::GetParameterlessStorageImpl( } void StorageManager::RegisterParametricStorageImpl( - TypeId type_id, std::function destructor) { + TypeId type_id, std::function destroy) { std::lock_guard guard(parametric_instance_lock_); VLOG(4) << "Register a parametric storage of: [TypeId_hash=" << std::hash()(type_id) << "]."; parametric_instance_.emplace( - type_id, std::make_unique(destructor)); + type_id, std::make_unique(destroy)); } void StorageManager::RegisterParameterlessStorageImpl( diff --git a/paddle/ir/core/storage_manager.h b/paddle/ir/core/storage_manager.h index b52d538e7dd26e..9ff7f76e98def5 100644 --- a/paddle/ir/core/storage_manager.h +++ b/paddle/ir/core/storage_manager.h @@ -98,25 +98,8 @@ class StorageManager { /// /// \param type_id The type id of the AbstractType. /// - template - struct IsTriviallyDestructible { - static bool call() { return false; } - }; - - template - struct IsTriviallyDestructible< - Storage, - typename std::enable_if< - std::is_trivially_destructible::value>::type> { - static bool call() { return true; } - }; - template void RegisterParametricStorage(TypeId type_id) { - if (IsTriviallyDestructible::call()) { - return RegisterParametricStorageImpl( - type_id, [](StorageBase *storage) { delete storage; }); - } return RegisterParametricStorageImpl(type_id, [](StorageBase *storage) { delete static_cast(storage); }); @@ -149,7 +132,7 @@ class StorageManager { StorageBase *GetParameterlessStorageImpl(TypeId type_id); void RegisterParametricStorageImpl( - TypeId type_id, std::function destructor); + TypeId type_id, std::function destroy); void RegisterParameterlessStorageImpl( TypeId type_id, std::function constructor); From c405f672e35b183ee12501099782f3595ed69765 Mon Sep 17 00:00:00 2001 From: zhangbo9674 Date: Fri, 9 Jun 2023 12:03:41 +0000 Subject: [PATCH 4/6] fix bug --- paddle/ir/core/storage_manager.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/paddle/ir/core/storage_manager.cc b/paddle/ir/core/storage_manager.cc index 24113ddc2725ad..6427d1f2e6f1cb 100644 --- a/paddle/ir/core/storage_manager.cc +++ b/paddle/ir/core/storage_manager.cc @@ -30,7 +30,7 @@ struct ParametricStorageManager { ~ParametricStorageManager() { IR_ENFORCE( - destroy_, + destroy_ != nullptr, "The desturctor of ParametricStorageManager should not be nullptr."); for (const auto &instance : parametric_instances_) { destroy_(instance.second); From 00199cb0aa9d3a180b7127321a643d34650d0914 Mon Sep 17 00:00:00 2001 From: zhangbo9674 Date: Fri, 9 Jun 2023 12:23:39 +0000 Subject: [PATCH 5/6] refine code --- paddle/ir/core/enforce.h | 2 +- paddle/ir/core/storage_manager.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/paddle/ir/core/enforce.h b/paddle/ir/core/enforce.h index e87ac0c41a07ee..d710fb248e038e 100644 --- a/paddle/ir/core/enforce.h +++ b/paddle/ir/core/enforce.h @@ -55,7 +55,7 @@ class IrNotMetException : public std::exception { #define IR_ENFORCE(COND, ...) \ do { \ - auto __cond__ = (COND); \ + bool __cond__(COND); \ if (UNLIKELY(is_error(__cond__))) { \ try { \ throw ir::IrNotMetException( \ diff --git a/paddle/ir/core/storage_manager.cc b/paddle/ir/core/storage_manager.cc index 6427d1f2e6f1cb..24113ddc2725ad 100644 --- a/paddle/ir/core/storage_manager.cc +++ b/paddle/ir/core/storage_manager.cc @@ -30,7 +30,7 @@ struct ParametricStorageManager { ~ParametricStorageManager() { IR_ENFORCE( - destroy_ != nullptr, + destroy_, "The desturctor of ParametricStorageManager should not be nullptr."); for (const auto &instance : parametric_instances_) { destroy_(instance.second); From f2ef0705e83fe829b42c64dd69d9a54c3f099c30 Mon Sep 17 00:00:00 2001 From: zhangbo9674 Date: Fri, 9 Jun 2023 12:55:03 +0000 Subject: [PATCH 6/6] refine code --- paddle/ir/core/enforce.h | 6 ++++-- paddle/ir/core/storage_manager.cc | 3 --- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/paddle/ir/core/enforce.h b/paddle/ir/core/enforce.h index d710fb248e038e..10735297f305d5 100644 --- a/paddle/ir/core/enforce.h +++ b/paddle/ir/core/enforce.h @@ -25,8 +25,10 @@ // there is no equivalent intrinsics in msvc. #define UNLIKELY(condition) (condition) #endif - -inline bool is_error(bool stat) { return !stat; } +template +inline bool is_error(const T& stat) { + return !stat; +} namespace ir { class IrNotMetException : public std::exception { diff --git a/paddle/ir/core/storage_manager.cc b/paddle/ir/core/storage_manager.cc index 24113ddc2725ad..8385c3d1fe4598 100644 --- a/paddle/ir/core/storage_manager.cc +++ b/paddle/ir/core/storage_manager.cc @@ -29,9 +29,6 @@ struct ParametricStorageManager { : destroy_(destroy) {} ~ParametricStorageManager() { - IR_ENFORCE( - destroy_, - "The desturctor of ParametricStorageManager should not be nullptr."); for (const auto &instance : parametric_instances_) { destroy_(instance.second); }