[Offload] Add olGetSymbolInfo[Size]

[RossBrunton]

This mirrors the similar functions for other handles. The only
implemented info at the moment is the symbol's kind.

[llvmbot]

@llvm/pr-subscribers-offload

Author: Ross Brunton (RossBrunton)

Changes

This mirrors the similar functions for other handles. The only
implemented info at the moment is the symbol's kind.

---

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

6 Files Affected:

• (modified) offload/liboffload/API/Symbol.td (+54)
• (modified) offload/liboffload/src/OffloadImpl.cpp (+28)
• (modified) offload/unittests/OffloadAPI/CMakeLists.txt (+3-1)
• (modified) offload/unittests/OffloadAPI/common/Fixtures.hpp (+18-2)
• (added) offload/unittests/OffloadAPI/symbol/olGetSymbolInfo.cpp (+65)
• (added) offload/unittests/OffloadAPI/symbol/olGetSymbolInfoSize.cpp (+46)

diff --git a/offload/liboffload/API/Symbol.td b/offload/liboffload/API/Symbol.td
index 68ccbc4e68c4b..42d473c24febd 100644
--- a/offload/liboffload/API/Symbol.td
+++ b/offload/liboffload/API/Symbol.td
@@ -32,3 +32,57 @@ def : Function {
     ];
     let returns = [];
 }
+
+def : Enum {
+  let name = "ol_symbol_info_t";
+  let desc = "Supported symbol info.";
+  let is_typed = 1;
+  let etors = [
+    TaggedEtor<"KIND", "ol_symbol_kind_t", "The kind of this symbol.">
+  ];
+}
+
+def : Function {
+  let name = "olGetSymbolInfo";
+  let desc = "Queries the given property of the symbol.";
+  let details = [
+    "`olGetSymbolInfoSize` can be used to query the storage size "
+    "required for the given query."
+  ];
+  let params = [
+    Param<"ol_symbol_handle_t", "Event", "handle of the symbol", PARAM_IN>,
+    Param<"ol_symbol_info_t", "PropName", "type of the info to retrieve", PARAM_IN>,
+    Param<"size_t", "PropSize", "the number of bytes pointed to by PropValue.", PARAM_IN>,
+    TypeTaggedParam<"void*", "PropValue", "array of bytes holding the info. "
+      "If PropSize is not equal to or greater to the real number of bytes needed to return the info "
+      "then the OL_ERRC_INVALID_SIZE error is returned and PropValue is not used.", PARAM_OUT,
+      TypeInfo<"PropName" , "PropSize">>
+  ];
+  let returns = [
+    Return<"OL_ERRC_INVALID_SIZE", [
+      "`PropSize == 0`",
+      "If `PropSize` is less than the real number of bytes needed to return the info."
+    ]>,
+    Return<"OL_ERRC_SYMBOL_KIND", [
+      "If the requested info isn't applicable to the type of symbol."
+    ]>,
+    Return<"OL_ERRC_INVALID_SYMBOL">
+  ];
+}
+
+def : Function {
+  let name = "olGetSymbolInfoSize";
+  let desc = "Returns the storage size of the given symbol query.";
+  let details = [];
+  let params = [
+    Param<"ol_symbol_handle_t", "Event", "handle of the symbol", PARAM_IN>,
+    Param<"ol_symbol_info_t", "PropName", "type of the info to query", PARAM_IN>,
+    Param<"size_t*", "PropSizeRet", "pointer to the number of bytes required to store the query", PARAM_OUT>
+  ];
+  let returns = [
+    Return<"OL_ERRC_INVALID_SYMBOL">,
+    Return<"OL_ERRC_SYMBOL_KIND", [
+      "If the requested info isn't applicable to the type of symbol."
+    ]>,
+  ];
+}
diff --git a/offload/liboffload/src/OffloadImpl.cpp b/offload/liboffload/src/OffloadImpl.cpp
index b4a545e013d6f..5aefafb1e57ea 100644
--- a/offload/liboffload/src/OffloadImpl.cpp
+++ b/offload/liboffload/src/OffloadImpl.cpp
@@ -746,5 +746,33 @@ Error olGetGlobalVariable_impl(ol_program_handle_t Program,
   return Error::success();
 }
 
+Error olGetSymbolInfoImplDetail(ol_symbol_handle_t Symbol,
+                                ol_symbol_info_t PropName, size_t PropSize,
+                                void *PropValue, size_t *PropSizeRet) {
+  InfoWriter Info(PropSize, PropValue, PropSizeRet);
+
+  switch (PropName) {
+  case OL_SYMBOL_INFO_KIND:
+    return Info.write<ol_symbol_kind_t>(Symbol->Kind);
+  default:
+    return createOffloadError(ErrorCode::INVALID_ENUMERATION,
+                              "olGetSymbolInfo enum '%i' is invalid", PropName);
+  }
+
+  return Error::success();
+}
+
+Error olGetSymbolInfo_impl(ol_symbol_handle_t Symbol, ol_symbol_info_t PropName,
+                           size_t PropSize, void *PropValue) {
+
+  return olGetSymbolInfoImplDetail(Symbol, PropName, PropSize, PropValue,
+                                   nullptr);
+}
+
+Error olGetSymbolInfoSize_impl(ol_symbol_handle_t Symbol,
+                               ol_symbol_info_t PropName, size_t *PropSizeRet) {
+  return olGetSymbolInfoImplDetail(Symbol, PropName, 0, nullptr, PropSizeRet);
+}
+
 } // namespace offload
 } // namespace llvm
diff --git a/offload/unittests/OffloadAPI/CMakeLists.txt b/offload/unittests/OffloadAPI/CMakeLists.txt
index 5d734df635cb4..0d60fe379a837 100644
--- a/offload/unittests/OffloadAPI/CMakeLists.txt
+++ b/offload/unittests/OffloadAPI/CMakeLists.txt
@@ -43,4 +43,6 @@ add_offload_unittest("queue"
     queue/olGetQueueInfoSize.cpp)
 
 add_offload_unittest("symbol"
-    symbol/olGetGlobalVariable.cpp)
+    symbol/olGetGlobalVariable.cpp
+    symbol/olGetSymbolInfo.cpp
+    symbol/olGetSymbolInfoSize.cpp)
diff --git a/offload/unittests/OffloadAPI/common/Fixtures.hpp b/offload/unittests/OffloadAPI/common/Fixtures.hpp
index e443d9761f30b..28f310a316a9c 100644
--- a/offload/unittests/OffloadAPI/common/Fixtures.hpp
+++ b/offload/unittests/OffloadAPI/common/Fixtures.hpp
@@ -91,9 +91,12 @@ struct OffloadPlatformTest : OffloadDeviceTest {
 // Fixture for a generic program test. If you want a different program, use
 // offloadQueueTest and create your own program handle with the binary you want.
 struct OffloadProgramTest : OffloadDeviceTest {
-  void SetUp() override {
+  void SetUp() override { SetUpWith("foo"); }
+
+  void SetUpWith(const char *ProgramName) {
     RETURN_ON_FATAL_FAILURE(OffloadDeviceTest::SetUp());
-    ASSERT_TRUE(TestEnvironment::loadDeviceBinary("foo", Device, DeviceBin));
+    ASSERT_TRUE(
+        TestEnvironment::loadDeviceBinary(ProgramName, Device, DeviceBin));
     ASSERT_GE(DeviceBin->getBufferSize(), 0lu);
     ASSERT_SUCCESS(olCreateProgram(Device, DeviceBin->getBufferStart(),
                                    DeviceBin->getBufferSize(), &Program));
@@ -123,6 +126,19 @@ struct OffloadKernelTest : OffloadProgramTest {
   ol_symbol_handle_t Kernel = nullptr;
 };
 
+struct OffloadGlobalTest : OffloadProgramTest {
+  void SetUp() override {
+    RETURN_ON_FATAL_FAILURE(OffloadProgramTest::SetUpWith("global"));
+    ASSERT_SUCCESS(olGetGlobalVariable(Program, "global", &Global));
+  }
+
+  void TearDown() override {
+    RETURN_ON_FATAL_FAILURE(OffloadProgramTest::TearDown());
+  }
+
+  ol_symbol_handle_t Global = nullptr;
+};
+
 struct OffloadQueueTest : OffloadDeviceTest {
   void SetUp() override {
     RETURN_ON_FATAL_FAILURE(OffloadDeviceTest::SetUp());
diff --git a/offload/unittests/OffloadAPI/symbol/olGetSymbolInfo.cpp b/offload/unittests/OffloadAPI/symbol/olGetSymbolInfo.cpp
new file mode 100644
index 0000000000000..100a374430372
--- /dev/null
+++ b/offload/unittests/OffloadAPI/symbol/olGetSymbolInfo.cpp
@@ -0,0 +1,65 @@
+//===------- Offload API tests - olGetSymbolInfo --------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <OffloadAPI.h>
+
+#include "../common/Fixtures.hpp"
+
+using olGetSymbolInfoKernelTest = OffloadKernelTest;
+OFFLOAD_TESTS_INSTANTIATE_DEVICE_FIXTURE(olGetSymbolInfoKernelTest);
+
+using olGetSymbolInfoGlobalTest = OffloadGlobalTest;
+OFFLOAD_TESTS_INSTANTIATE_DEVICE_FIXTURE(olGetSymbolInfoGlobalTest);
+
+TEST_P(olGetSymbolInfoKernelTest, SuccessKind) {
+  ol_symbol_kind_t RetrievedKind;
+  ASSERT_SUCCESS(olGetSymbolInfo(Kernel, OL_SYMBOL_INFO_KIND,
+                                 sizeof(RetrievedKind), &RetrievedKind));
+  ASSERT_EQ(RetrievedKind, OL_SYMBOL_KIND_KERNEL);
+}
+
+TEST_P(olGetSymbolInfoGlobalTest, SuccessKind) {
+  ol_symbol_kind_t RetrievedKind;
+  ASSERT_SUCCESS(olGetSymbolInfo(Global, OL_SYMBOL_INFO_KIND,
+                                 sizeof(RetrievedKind), &RetrievedKind));
+  ASSERT_EQ(RetrievedKind, OL_SYMBOL_KIND_GLOBAL_VARIABLE);
+}
+
+TEST_P(olGetSymbolInfoKernelTest, InvalidNullHandle) {
+  ol_symbol_kind_t RetrievedKind;
+  ASSERT_ERROR(OL_ERRC_INVALID_NULL_HANDLE,
+               olGetSymbolInfo(nullptr, OL_SYMBOL_INFO_KIND,
+                               sizeof(RetrievedKind), &RetrievedKind));
+}
+
+TEST_P(olGetSymbolInfoKernelTest, InvalidSymbolInfoEnumeration) {
+  ol_symbol_kind_t RetrievedKind;
+  ASSERT_ERROR(OL_ERRC_INVALID_ENUMERATION,
+               olGetSymbolInfo(Kernel, OL_SYMBOL_INFO_FORCE_UINT32,
+                               sizeof(RetrievedKind), &RetrievedKind));
+}
+
+TEST_P(olGetSymbolInfoKernelTest, InvalidSizeZero) {
+  ol_symbol_kind_t RetrievedKind;
+  ASSERT_ERROR(OL_ERRC_INVALID_SIZE,
+               olGetSymbolInfo(Kernel, OL_SYMBOL_INFO_KIND, 0, &RetrievedKind));
+}
+
+TEST_P(olGetSymbolInfoKernelTest, InvalidSizeSmall) {
+  ol_symbol_kind_t RetrievedKind;
+  ASSERT_ERROR(OL_ERRC_INVALID_SIZE,
+               olGetSymbolInfo(Kernel, OL_SYMBOL_INFO_KIND,
+                               sizeof(RetrievedKind) - 1, &RetrievedKind));
+}
+
+TEST_P(olGetSymbolInfoKernelTest, InvalidNullPointerPropValue) {
+  ol_symbol_kind_t RetrievedKind;
+  ASSERT_ERROR(OL_ERRC_INVALID_NULL_POINTER,
+               olGetSymbolInfo(Kernel, OL_SYMBOL_INFO_KIND,
+                               sizeof(RetrievedKind), nullptr));
+}
diff --git a/offload/unittests/OffloadAPI/symbol/olGetSymbolInfoSize.cpp b/offload/unittests/OffloadAPI/symbol/olGetSymbolInfoSize.cpp
new file mode 100644
index 0000000000000..aa7a061a9ef7a
--- /dev/null
+++ b/offload/unittests/OffloadAPI/symbol/olGetSymbolInfoSize.cpp
@@ -0,0 +1,46 @@
+//===------- Offload API tests - olGetSymbolInfoSize ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include <OffloadAPI.h>
+
+#include "../common/Fixtures.hpp"
+
+using olGetSymbolInfoSizeKernelTest = OffloadKernelTest;
+OFFLOAD_TESTS_INSTANTIATE_DEVICE_FIXTURE(olGetSymbolInfoSizeKernelTest);
+
+using olGetSymbolInfoSizeGlobalTest = OffloadGlobalTest;
+OFFLOAD_TESTS_INSTANTIATE_DEVICE_FIXTURE(olGetSymbolInfoSizeGlobalTest);
+
+TEST_P(olGetSymbolInfoSizeKernelTest, SuccessKind) {
+  size_t Size = 0;
+  ASSERT_SUCCESS(olGetSymbolInfoSize(Kernel, OL_SYMBOL_INFO_KIND, &Size));
+  ASSERT_EQ(Size, sizeof(ol_symbol_kind_t));
+}
+
+TEST_P(olGetSymbolInfoSizeGlobalTest, SuccessKind) {
+  size_t Size = 0;
+  ASSERT_SUCCESS(olGetSymbolInfoSize(Global, OL_SYMBOL_INFO_KIND, &Size));
+  ASSERT_EQ(Size, sizeof(ol_symbol_kind_t));
+}
+
+TEST_P(olGetSymbolInfoSizeKernelTest, InvalidNullHandle) {
+  size_t Size = 0;
+  ASSERT_ERROR(OL_ERRC_INVALID_NULL_HANDLE,
+               olGetSymbolInfoSize(nullptr, OL_SYMBOL_INFO_KIND, &Size));
+}
+
+TEST_P(olGetSymbolInfoSizeKernelTest, InvalidSymbolInfoEnumeration) {
+  size_t Size = 0;
+  ASSERT_ERROR(OL_ERRC_INVALID_ENUMERATION,
+               olGetSymbolInfoSize(Kernel, OL_SYMBOL_INFO_FORCE_UINT32, &Size));
+}
+
+TEST_P(olGetSymbolInfoSizeKernelTest, InvalidNullPointer) {
+  ASSERT_ERROR(OL_ERRC_INVALID_NULL_POINTER,
+               olGetSymbolInfoSize(Kernel, OL_SYMBOL_INFO_KIND, nullptr));
+}

[jhuber6]

What do we use the Size variant for exactly? I figured that would just be part of the query, most documentation will state how big the output variable needs to be when using some of these enums.

[RossBrunton]

@jhuber6 Some queries (such as name and vendor) return strings, binary data or arrays. This entry point allows the implementation to pre-allocate storage for them.

It's also useful for offload users that want to create a generic "readInfo" function that allocates and returns a void * to some memory for callers to cast to the appropriate type.

[jhuber6]

@jhuber6 Some queries (such as name and vendor) return strings, binary data or arrays. This entry point allows the implementation to pre-allocate storage for them.

It's also useful for offload users that want to create a generic "readInfo" function that allocates and returns a void * to some memory for callers to cast to the appropriate type.

I assumed those would just return a pointer to some memory the plugin manages, likely the assumption is that a symbol is valid only as long as its associated image is alive... Guess this is another case where the runtime really should be managing the memory of the image by copying it in. I really need to make that change.

[jhuber6]

I would prefer that we do this like HSA does and have HSA_EXECUTABLE_SYMBOL_INFO_NAME_LENGTH as a separate info that you can query first and then use as a pointer for HSA_EXECUTABLE_SYMBOL_INFO_NAME.

[RossBrunton]

@jhuber6 Given that this interface matches the interface of other handles, any change to how it fundamentally works should probably involve updating all other getInfo queries. If we do decide to replace the Size variants, I think that should be done as a separate task that touches everything.

For now, I think it makes sense to match other handle points, and leave any refactors for device info as a separate change that touches everything.

[jhuber6]

@jhuber6 Given that this interface matches the interface of other handles, any change to how it fundamentally works should probably involve updating all other getInfo queries. If we do decide to replace the Size variants, I think that should be done as a separate task that touches everything.

For now, I think it makes sense to match other handle points, and leave any refactors for device info as a separate change that touches everything.

Sure, we can get in and then rework all the get info's to just have a separate query for the size. I think that's much cleaner and keeps the number of API functions more clear. I'll accept this for now but we should definitely do that later.