[Offload] Make MemoryManager threshold ENV var size_t type.

[hidekisaito]

No description provided.

[llvmbot]

@llvm/pr-subscribers-offload

Author: None (hidekisaito)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/124063.diff

2 Files Affected:

• (modified) offload/include/Shared/EnvironmentVar.h (+1)
• (modified) offload/plugins-nextgen/common/include/MemoryManager.h (+1-1)

diff --git a/offload/include/Shared/EnvironmentVar.h b/offload/include/Shared/EnvironmentVar.h
index 82f434e91a85b8..c1854774d572b6 100644
--- a/offload/include/Shared/EnvironmentVar.h
+++ b/offload/include/Shared/EnvironmentVar.h
@@ -128,6 +128,7 @@ using Int32Envar = Envar<int32_t>;
 using Int64Envar = Envar<int64_t>;
 using UInt32Envar = Envar<uint32_t>;
 using UInt64Envar = Envar<uint64_t>;
+using SizeTEnvar = Envar<size_t>;
 using StringEnvar = Envar<std::string>;
 using BoolEnvar = Envar<bool>;
 
diff --git a/offload/plugins-nextgen/common/include/MemoryManager.h b/offload/plugins-nextgen/common/include/MemoryManager.h
index fe1989930b76ef..34e9e9618680f3 100644
--- a/offload/plugins-nextgen/common/include/MemoryManager.h
+++ b/offload/plugins-nextgen/common/include/MemoryManager.h
@@ -324,7 +324,7 @@ class MemoryManagerTy {
   /// manager explicitly by setting the var to 0. If user doesn't specify
   /// anything, returns <0, true>.
   static std::pair<size_t, bool> getSizeThresholdFromEnv() {
-    static UInt32Envar MemoryManagerThreshold(
+    static SizeTEnvar MemoryManagerThreshold(
         "LIBOMPTARGET_MEMORY_MANAGER_THRESHOLD", 0);
 
     size_t Threshold = MemoryManagerThreshold.get();

[jhuber6]

Can we use uint64_t instead?

[hidekisaito]

Can we use uint64_t instead?

First change I thought of was indeed the use of UInt64Envar, but the variable being assigned to
is size_t and the actual use of the value in the MemoryManager constructor is also size_t.
So, I thought (and still think) it is more prudent to use size_t. If size_t sized ENV var isn't
a great idea, I'd like to learn why and will change my PR back to UInt64Envar.

[jhuber6]

Can we use uint64_t instead?

First change I thought of was indeed the use of UInt64Envar, but the variable being assigned to is size_t and the actual use of the value in the MemoryManager constructor is also size_t. So, I thought (and still think) it is more prudent to use size_t. If size_t sized ENV var isn't a great idea, I'd like to learn why and will change my PR back to UInt64Envar.

We don't support any 32-bit platforms at all right now, and generally I don't like size_t because it varies by platform. It's just much easier to make it 64-bit and call it a day without worrying about how behavior changes with whatever build.

[hidekisaito]

I'm a big fan of consistency in the coding. Looks like we need another set of eyes to break a tie.

@jdoerfert (author of 0783bf1), between UInt64Envar and newly creating SizeTEnvar, which would you prefer? I'd like to let users choose/try 4+GB threshold if desired.

[hidekisaito]

In the interest of moving this quicker, I changed it to use UInt64Envar instead. I can try create/use SizeTEnvar after this patch goes in.

@jhuber6, thanks for the approval. When the test passes, please merge.

[shiltian]

size_t is more for platform portable. We decided to not support 32-bit at some point in the past so using uint64_t is fine.