Revert "[OFFLOAD][OPENMP] 6.0 compatible interop interface (#143491)" #161279
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…)" This reverts commit 66d1c37. This patch is the runtime library part of a 2-patch piece of work. The second piece to this is codegen work. For the codegen part, there currently is no clear timeline. However, without the codegen part, this patch breaks interop and does not make sense. I propose to revert this patch for now and clarify the path forward.
jplehr
requested review from
mjklemm,
jdoerfert,
kevinsala,
dpalermo,
abhinavgaba and
jhuber6
|
@llvm/pr-subscribers-offload Author: Jan Patrick Lehr (jplehr) ChangesThis reverts commit 66d1c37. This patch is the runtime library part of a 2-patch piece of work. The second piece to this is codegen work. For the codegen part, there currently is no clear timeline. However, without the codegen part, this patch breaks interop and does not make sense. I propose to revert this patch for now and clarify the path forward. Patch is 48.45 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/161279.diff 16 Files Affected:
diff --git a/offload/include/OpenMP/InteropAPI.h b/offload/include/OpenMP/InteropAPI.h
index 53ac4be2e2e98..71c78760a3226 100644
--- a/offload/include/OpenMP/InteropAPI.h
+++ b/offload/include/OpenMP/InteropAPI.h
@@ -13,69 +13,17 @@
#include "omp.h"
-#include "PerThreadTable.h"
#include "omptarget.h"
extern "C" {
typedef enum kmp_interop_type_t {
kmp_interop_type_unknown = -1,
- kmp_interop_type_target,
- kmp_interop_type_targetsync,
+ kmp_interop_type_platform,
+ kmp_interop_type_device,
+ kmp_interop_type_tasksync,
} kmp_interop_type_t;
-struct interop_attrs_t {
- bool inorder : 1;
- int reserved : 31;
-
- /// Check if the supported attributes are compatible with the current
- /// attributes. Only if an attribute is supported can the value be true,
- /// otherwise it needs to be false
- bool checkSupportedOnly(interop_attrs_t supported) const {
- return supported.inorder || (!supported.inorder && !inorder);
- }
-};
-
-struct interop_spec_t {
- int32_t fr_id;
- interop_attrs_t attrs; // Common attributes
- int64_t impl_attrs; // Implementation specific attributes (recognized by each
- // plugin)
-};
-
-struct interop_flags_t {
- bool implicit : 1; // dispatch (true) or interop (false)
- bool nowait : 1; // has nowait flag
- int reserved : 30;
-};
-
-struct interop_ctx_t {
- uint32_t version; // version of the interface (current is 0)
- interop_flags_t flags;
- int gtid;
-};
-
-struct dep_pack_t {
- int32_t ndeps;
- int32_t ndeps_noalias;
- kmp_depend_info_t *deplist;
- kmp_depend_info_t *noalias_deplist;
-};
-
-struct omp_interop_val_t;
-
-typedef void ompx_interop_cb_t(omp_interop_val_t *interop, void *data);
-
-struct omp_interop_cb_instance_t {
- ompx_interop_cb_t *cb;
- void *data;
-
- omp_interop_cb_instance_t(ompx_interop_cb_t *cb, void *data)
- : cb(cb), data(data) {}
-
- void operator()(omp_interop_val_t *interop) { cb(interop, data); }
-};
-
/// The interop value type, aka. the interop object.
typedef struct omp_interop_val_t {
/// Device and interop-type are determined at construction time and fix.
@@ -86,98 +34,10 @@ typedef struct omp_interop_val_t {
__tgt_device_info device_info;
const kmp_interop_type_t interop_type;
const intptr_t device_id;
- omp_vendor_id_t vendor_id = omp_vendor_llvm;
- tgt_foreign_runtime_id_t fr_id = tgt_fr_none;
- interop_attrs_t attrs{false, 0}; // Common prefer specification attributes
- int64_t impl_attrs = 0; // Implementation prefer specification attributes
-
- // Constants
- static constexpr int no_owner = -1; // This interop has no current owner
-
- void *rtl_property = nullptr; // Plugin dependent information
- // For implicitly created Interop objects (e.g., from a dispatch construct)
- // who owns the object
- int owner_gtid = no_owner;
- // Marks whether the object was requested since the last time it was synced
- bool clean = true;
-
- typedef llvm::SmallVector<omp_interop_cb_instance_t> callback_list_t;
-
- callback_list_t completion_cbs;
-
- void reset() {
- owner_gtid = no_owner;
- markClean();
- clearCompletionCbs();
- }
-
- llvm::Expected<DeviceTy &> getDevice() const;
-
- bool hasOwner() const { return owner_gtid != no_owner; }
-
- void setOwner(int gtid) { owner_gtid = gtid; }
- bool isOwnedBy(int gtid) { return owner_gtid == gtid; }
- bool isCompatibleWith(int32_t InteropType, const interop_spec_t &Spec);
- bool isCompatibleWith(int32_t InteropType, const interop_spec_t &Spec,
- int64_t DeviceNum, int gtid);
- void markClean() { clean = true; }
- void markDirty() { clean = false; }
- bool isClean() const { return clean; }
-
- int32_t flush(DeviceTy &Device);
- int32_t sync_barrier(DeviceTy &Device);
- int32_t async_barrier(DeviceTy &Device);
- int32_t release(DeviceTy &Device);
-
- void addCompletionCb(ompx_interop_cb_t *cb, void *data) {
- completion_cbs.push_back(omp_interop_cb_instance_t(cb, data));
- }
-
- int numCompletionCbs() const { return completion_cbs.size(); }
- void clearCompletionCbs() { completion_cbs.clear(); }
-
- void runCompletionCbs() {
- for (auto &cbInstance : completion_cbs)
- cbInstance(this);
- clearCompletionCbs();
- }
+ const omp_foreign_runtime_ids_t vendor_id = cuda;
+ const intptr_t backend_type_id = omp_interop_backend_type_cuda_1;
} omp_interop_val_t;
} // extern "C"
-struct InteropTableEntry {
- using ContainerTy = typename std::vector<omp_interop_val_t *>;
- using iterator = typename ContainerTy::iterator;
-
- ContainerTy Interops;
-
- static constexpr int reservedEntriesPerThread =
- 20; // reserve some entries to avoid reallocation
-
- void add(omp_interop_val_t *obj) {
- if (Interops.capacity() == 0)
- Interops.reserve(reservedEntriesPerThread);
- Interops.push_back(obj);
- }
-
- template <class ClearFuncTy> void clear(ClearFuncTy f) {
- for (auto &Obj : Interops) {
- f(Obj);
- }
- }
-
- /// vector interface
- int size() const { return Interops.size(); }
- iterator begin() { return Interops.begin(); }
- iterator end() { return Interops.end(); }
- iterator erase(iterator it) { return Interops.erase(it); }
-};
-
-struct InteropTblTy
- : public PerThreadTable<InteropTableEntry, omp_interop_val_t *> {
- void clear();
-};
-
-void syncImplicitInterops(int gtid, void *event);
-
#endif // OMPTARGET_OPENMP_INTEROP_API_H
diff --git a/offload/include/OpenMP/omp.h b/offload/include/OpenMP/omp.h
index 49d9f1fa75c20..b44c6aff1b289 100644
--- a/offload/include/OpenMP/omp.h
+++ b/offload/include/OpenMP/omp.h
@@ -80,18 +80,15 @@ typedef enum omp_interop_rc {
omp_irc_other = -6
} omp_interop_rc_t;
-/* Foreign runtime values from OpenMP Additional Definitions document v2.1 */
-typedef enum tgt_foreign_runtime_id_t {
- tgt_fr_none = 0,
- tgt_fr_cuda = 1,
- tgt_fr_cuda_driver = 2,
- tgt_fr_opencl = 3,
- tgt_fr_sycl = 4,
- tgt_fr_hip = 5,
- tgt_fr_level_zero = 6,
- tgt_fr_hsa = 7,
- tgt_fr_last = 8
-} tgt_foreign_runtime_id_t;
+typedef enum omp_interop_fr {
+ omp_ifr_cuda = 1,
+ omp_ifr_cuda_driver = 2,
+ omp_ifr_opencl = 3,
+ omp_ifr_sycl = 4,
+ omp_ifr_hip = 5,
+ omp_ifr_level_zero = 6,
+ omp_ifr_last = 7
+} omp_interop_fr_t;
typedef void *omp_interop_t;
@@ -137,23 +134,19 @@ omp_get_interop_type_desc(const omp_interop_t, omp_interop_property_t);
extern const char *__KAI_KMPC_CONVENTION
omp_get_interop_rc_desc(const omp_interop_t, omp_interop_rc_t);
-/* Vendor defined values from OpenMP Additional Definitions document v2.1*/
-typedef enum omp_vendor_id {
- omp_vendor_unknown = 0,
- omp_vendor_amd = 1,
- omp_vendor_arm = 2,
- omp_vendor_bsc = 3,
- omp_vendor_fujitsu = 4,
- omp_vendor_gnu = 5,
- omp_vendor_hpe = 6,
- omp_vendor_ibm = 7,
- omp_vendor_intel = 8,
- omp_vendor_llvm = 9,
- omp_vendor_nec = 10,
- omp_vendor_nvidia = 11,
- omp_vendor_ti = 12,
- omp_vendor_last = 13
-} omp_vendor_id_t;
+typedef enum omp_interop_backend_type_t {
+ // reserve 0
+ omp_interop_backend_type_cuda_1 = 1,
+} omp_interop_backend_type_t;
+
+typedef enum omp_foreign_runtime_ids {
+ cuda = 1,
+ cuda_driver = 2,
+ opencl = 3,
+ sycl = 4,
+ hip = 5,
+ level_zero = 6,
+} omp_foreign_runtime_ids_t;
///} InteropAPI
diff --git a/offload/include/PerThreadTable.h b/offload/include/PerThreadTable.h
deleted file mode 100644
index 45b196171b4c8..0000000000000
--- a/offload/include/PerThreadTable.h
+++ /dev/null
@@ -1,114 +0,0 @@
-//===-- PerThreadTable.h -- PerThread Storage Structure ----*- C++ -*-===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-//
-// Table indexed with one entry per thread.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef OFFLOAD_PERTHREADTABLE_H
-#define OFFLOAD_PERTHREADTABLE_H
-
-#include <list>
-#include <memory>
-#include <mutex>
-
-// Using an STL container (such as std::vector) indexed by thread ID has
-// too many race conditions issues so we store each thread entry into a
-// thread_local variable.
-// T is the container type used to store the objects, e.g., std::vector,
-// std::set, etc. by each thread. O is the type of the stored objects e.g.,
-// omp_interop_val_t *, ...
-
-template <typename ContainerType, typename ObjectType> struct PerThreadTable {
- using iterator = typename ContainerType::iterator;
-
- struct PerThreadData {
- size_t NElements = 0;
- std::unique_ptr<ContainerType> ThEntry;
- };
-
- std::mutex Mtx;
- std::list<std::shared_ptr<PerThreadData>> ThreadDataList;
-
- // define default constructors, disable copy and move constructors
- PerThreadTable() = default;
- PerThreadTable(const PerThreadTable &) = delete;
- PerThreadTable(PerThreadTable &&) = delete;
- PerThreadTable &operator=(const PerThreadTable &) = delete;
- PerThreadTable &operator=(PerThreadTable &&) = delete;
- ~PerThreadTable() {
- std::lock_guard<std::mutex> Lock(Mtx);
- ThreadDataList.clear();
- }
-
-private:
- PerThreadData &getThreadData() {
- static thread_local std::shared_ptr<PerThreadData> ThData = nullptr;
- if (!ThData) {
- ThData = std::make_shared<PerThreadData>();
- std::lock_guard<std::mutex> Lock(Mtx);
- ThreadDataList.push_back(ThData);
- }
- return *ThData;
- }
-
-protected:
- ContainerType &getThreadEntry() {
- auto &ThData = getThreadData();
- if (ThData.ThEntry)
- return *ThData.ThEntry;
- ThData.ThEntry = std::make_unique<ContainerType>();
- return *ThData.ThEntry;
- }
-
- size_t &getThreadNElements() {
- auto &ThData = getThreadData();
- return ThData.NElements;
- }
-
-public:
- void add(ObjectType obj) {
- auto &Entry = getThreadEntry();
- auto &NElements = getThreadNElements();
- NElements++;
- Entry.add(obj);
- }
-
- iterator erase(iterator it) {
- auto &Entry = getThreadEntry();
- auto &NElements = getThreadNElements();
- NElements--;
- return Entry.erase(it);
- }
-
- size_t size() { return getThreadNElements(); }
-
- // Iterators to traverse objects owned by
- // the current thread
- iterator begin() {
- auto &Entry = getThreadEntry();
- return Entry.begin();
- }
- iterator end() {
- auto &Entry = getThreadEntry();
- return Entry.end();
- }
-
- template <class F> void clear(F f) {
- std::lock_guard<std::mutex> Lock(Mtx);
- for (auto ThData : ThreadDataList) {
- if (!ThData->ThEntry || ThData->NElements == 0)
- continue;
- ThData->ThEntry->clear(f);
- ThData->NElements = 0;
- }
- ThreadDataList.clear();
- }
-};
-
-#endif
diff --git a/offload/include/PluginManager.h b/offload/include/PluginManager.h
index 6c6fdebe76dff..ec3adadf0819b 100644
--- a/offload/include/PluginManager.h
+++ b/offload/include/PluginManager.h
@@ -35,8 +35,6 @@
#include <mutex>
#include <string>
-#include "OpenMP/InteropAPI.h"
-
using GenericPluginTy = llvm::omp::target::plugin::GenericPluginTy;
/// Struct for the data required to handle plugins
@@ -90,9 +88,6 @@ struct PluginManager {
HostPtrToTableMapTy HostPtrToTableMap;
std::mutex TblMapMtx; ///< For HostPtrToTableMap
- /// Table of cached implicit interop objects
- InteropTblTy InteropTbl;
-
// Work around for plugins that call dlopen on shared libraries that call
// tgt_register_lib during their initialisation. Stash the pointers in a
// vector until the plugins are all initialised and then register them.
@@ -190,6 +185,5 @@ void initRuntime();
void deinitRuntime();
extern PluginManager *PM;
-extern std::atomic<bool> RTLAlive; // Indicates if the RTL has been initialized
-extern std::atomic<int> RTLOngoingSyncs; // Counts ongoing external syncs
+
#endif // OMPTARGET_PLUGIN_MANAGER_H
diff --git a/offload/include/Shared/APITypes.h b/offload/include/Shared/APITypes.h
index 8c150b6bfc2d4..1982043bd9ef4 100644
--- a/offload/include/Shared/APITypes.h
+++ b/offload/include/Shared/APITypes.h
@@ -37,7 +37,6 @@ struct __tgt_device_image {
struct __tgt_device_info {
void *Context = nullptr;
void *Device = nullptr;
- void *Platform = nullptr;
};
/// This struct is a record of all the host code that may be offloaded to a
diff --git a/offload/libomptarget/OffloadRTL.cpp b/offload/libomptarget/OffloadRTL.cpp
index 04bd21ec91a49..29b573a27d087 100644
--- a/offload/libomptarget/OffloadRTL.cpp
+++ b/offload/libomptarget/OffloadRTL.cpp
@@ -22,8 +22,6 @@ extern void llvm::omp::target::ompt::connectLibrary();
static std::mutex PluginMtx;
static uint32_t RefCount = 0;
-std::atomic<bool> RTLAlive{false};
-std::atomic<int> RTLOngoingSyncs{0};
void initRuntime() {
std::scoped_lock<decltype(PluginMtx)> Lock(PluginMtx);
@@ -43,9 +41,6 @@ void initRuntime() {
PM->init();
PM->registerDelayedLibraries();
-
- // RTL initialization is complete
- RTLAlive = true;
}
}
@@ -55,13 +50,6 @@ void deinitRuntime() {
if (RefCount == 1) {
DP("Deinit offload library!\n");
- // RTL deinitialization has started
- RTLAlive = false;
- while (RTLOngoingSyncs > 0) {
- DP("Waiting for ongoing syncs to finish, count: %d\n",
- RTLOngoingSyncs.load());
- std::this_thread::sleep_for(std::chrono::milliseconds(100));
- }
PM->deinit();
delete PM;
PM = nullptr;
diff --git a/offload/libomptarget/OpenMP/API.cpp b/offload/libomptarget/OpenMP/API.cpp
index b0f0573833713..4576f9bd06121 100644
--- a/offload/libomptarget/OpenMP/API.cpp
+++ b/offload/libomptarget/OpenMP/API.cpp
@@ -16,7 +16,6 @@
#include "rtl.h"
#include "OpenMP/InternalTypes.h"
-#include "OpenMP/InteropAPI.h"
#include "OpenMP/Mapping.h"
#include "OpenMP/OMPT/Interface.h"
#include "OpenMP/omp.h"
@@ -684,21 +683,3 @@ EXTERN void *omp_get_mapped_ptr(const void *Ptr, int DeviceNum) {
return TPR.TargetPointer;
}
-
-// This routine gets called from the Host RTL at sync points (taskwait, barrier,
-// ...) so we can synchronize the necessary objects from the offload side.
-EXTERN void __tgt_target_sync(ident_t *loc_ref, int gtid, void *current_task,
- void *event) {
- if (!RTLAlive)
- return;
-
- RTLOngoingSyncs++;
- if (!RTLAlive) {
- RTLOngoingSyncs--;
- return;
- }
-
- syncImplicitInterops(gtid, event);
-
- RTLOngoingSyncs--;
-}
diff --git a/offload/libomptarget/OpenMP/InteropAPI.cpp b/offload/libomptarget/OpenMP/InteropAPI.cpp
index eb5425ecbf062..bdbc440c64a2c 100644
--- a/offload/libomptarget/OpenMP/InteropAPI.cpp
+++ b/offload/libomptarget/OpenMP/InteropAPI.cpp
@@ -10,7 +10,6 @@
#include "OpenMP/InternalTypes.h"
#include "OpenMP/omp.h"
-#include "OffloadPolicy.h"
#include "PluginManager.h"
#include "device.h"
#include "omptarget.h"
@@ -57,22 +56,22 @@ void getTypeMismatch(omp_interop_property_t Property, int *Err) {
*Err = getPropertyErrorType(Property);
}
-static const char *VendorStrTbl[] = {
- "unknown", "amd", "arm", "bsc", "fujitsu", "gnu", "hpe",
- "ibm", "intel", "llvm", "nec", "nvidia", "ti"};
-const char *getVendorIdToStr(const omp_vendor_id_t VendorId) {
- if (VendorId < omp_vendor_unknown || VendorId >= omp_vendor_last)
- return ("unknown");
- return VendorStrTbl[VendorId];
-}
-
-static const char *ForeignRuntimeStrTbl[] = {
- "none", "cuda", "cuda_driver", "opencl",
- "sycl", "hip", "level_zero", "hsa"};
-const char *getForeignRuntimeIdToStr(const tgt_foreign_runtime_id_t FrId) {
- if (FrId < tgt_fr_none || FrId >= tgt_fr_last)
- return ("unknown");
- return ForeignRuntimeStrTbl[FrId];
+const char *getVendorIdToStr(const omp_foreign_runtime_ids_t VendorId) {
+ switch (VendorId) {
+ case cuda:
+ return ("cuda");
+ case cuda_driver:
+ return ("cuda_driver");
+ case opencl:
+ return ("opencl");
+ case sycl:
+ return ("sycl");
+ case hip:
+ return ("hip");
+ case level_zero:
+ return ("level_zero");
+ }
+ return ("unknown");
}
template <typename PropertyTy>
@@ -84,7 +83,7 @@ intptr_t getProperty<intptr_t>(omp_interop_val_t &InteropVal,
omp_interop_property_t Property, int *Err) {
switch (Property) {
case omp_ipr_fr_id:
- return InteropVal.fr_id;
+ return InteropVal.backend_type_id;
case omp_ipr_vendor:
return InteropVal.vendor_id;
case omp_ipr_device_num:
@@ -100,8 +99,10 @@ const char *getProperty<const char *>(omp_interop_val_t &InteropVal,
omp_interop_property_t Property,
int *Err) {
switch (Property) {
- case omp_ipr_fr_name:
- return getForeignRuntimeIdToStr(InteropVal.fr_id);
+ case omp_ipr_fr_id:
+ return InteropVal.interop_type == kmp_interop_type_tasksync
+ ? "tasksync"
+ : "device+context";
case omp_ipr_vendor_name:
return getVendorIdToStr(InteropVal.vendor_id);
default:
@@ -119,8 +120,6 @@ void *getProperty<void *>(omp_interop_val_t &InteropVal,
return InteropVal.device_info.Device;
*Err = omp_irc_no_value;
return const_cast<char *>(InteropVal.err_str);
- case omp_ipr_platform:
- return InteropVal.device_info.Platform;
case omp_ipr_device_context:
return InteropVal.device_info.Context;
case omp_ipr_targetsync:
@@ -146,13 +145,13 @@ bool getPropertyCheck(omp_interop_val_t **InteropPtr,
return false;
}
if (Property == omp_ipr_targetsync &&
- (*InteropPtr)->interop_type != kmp_interop_type_targetsync) {
+ (*InteropPtr)->interop_type != kmp_interop_type_tasksync) {
if (Err)
*Err = omp_irc_other;
return false;
}
if ((Property == omp_ipr_device || Property == omp_ipr_device_context) &&
- (*InteropPtr)->interop_type == kmp_interop_type_targetsync) {
+ (*InteropPtr)->interop_type == kmp_interop_type_tasksync) {
if (Err)
*Err = omp_irc_other;
return false;
@@ -167,7 +166,7 @@ bool getPropertyCheck(omp_interop_val_t **InteropPtr,
omp_interop_property_t property_id, \
int *err) { \
omp_interop_val_t *interop_val = (omp_interop_val_t *)interop; \
- assert((interop_val)->interop_type == kmp_interop_type_targetsync); \
+ assert((interop_val)->interop_type == kmp_interop_type_tasksync); \
if (!getPropertyCheck(&interop_val, property_id, err)) { \
return (RETURN_TYPE)(0); \
} \
@@ -194,278 +193,119 @@ __OMP_GET_INTEROP_TY3(const char *, type_desc)
__OMP_GET_INTEROP_TY3(const char *, rc_desc)
#undef __OMP_GET_INTEROP_TY3
-extern "C" {
+static const char *copyErrorString(llvm::Error &&Err) {
+ // TODO: Use the error string while avoiding leaks.
+ std::string ErrMsg = llvm::toString(std::move(Err));
+ char *UsrMsg = reinterpret_cast<char *>(malloc(ErrMsg.size() + 1));
+ strcpy(UsrMsg, ErrMsg.c_str());
+ return UsrMsg;
+}
-omp_interop_val_t *__tgt_interop_get(ident_t *LocRef, int32_t InteropType,
- int64_t DeviceNum, int32_t NumPrefers,
- interop_spec_t *Prefers,
- interop_ctx_t *Ctx, dep_pack_t *Deps) {
-
- DP("Call to %s with device_num %" PRId64 ", interop type %" PRId32
- ", number of preferred specs %" PRId32 "%s%s\n",
- __func__, DeviceNum, InteropType, NumPrefers,
- Ctx->flags.implicit ? " (implicit)" : "",
- Ctx->flags.nowait ? " (nowait)" : "");
-
- if (OffloadPolicy::get(*PM).Kind == OffloadPolicy::DISABLED)
- return omp_interop_none;
-
- // Now, try to create an interop with device_num.
- if (DeviceNum == OFFLOAD_DEVICE_DEFAULT)
- DeviceNum = omp_get_default_device();
-
- auto gtid = Ctx->gtid;
-
- if (InteropType == kmp_interop_type_targetsync) {
- if (Ctx->flags.nowait)
- DP("Warning: nowait flag on interop creation not supported yet. "
- "Ignored\n");
- if (Deps)
- __kmpc_omp_wait_deps(LocRef, gtid, Deps->ndeps, Deps->deplist,
- Deps->ndeps_noalias, Deps->noalias_deplist);
- }
+extern "C...
[truncated]
|
|
I initially wanted to bring this to the OpenMP call, but I realized that it may be quite some time for the next call, given there is IWOMP and the OpenMP Spec meeting. As said in the description: The issue really is that without the codegen work, interop is simply broken and fails with linker errors. I don't think we should leave it in that state. I don't know the status of the codegen work, whether it has been started or how far it is, but with the current information at my disposal, IMHO, we should revert this, figure out what's left to do and then reland both in a coordinated way. Pulling in @adurang, @CatherineMoore and @dreachem for awareness. |
|
Does this make pass one of the tests I just disabled? #161265 |
|
CC @adurang |
Maybe on nvidia. On AMDGPU there is another issue. |
jhuber6
approved these changes
Sep 29, 2025
|
I'm gonna wait with landing some more to give time for others to jump into the discussion. |
|
Given that the previous RTL support didn't properly implement most of the interop functionality I personally think it's best to try to fix it from here. IMO, it would be better to just reinstate the old APIs as wrappers to the new APIs which would make the current code being generated work (albeit without being able to use the extended semantics). Init/destroy had distinct names so those are really ease, use is using the same name so the new API would need a new name but I don't thing that's a significant issue. It's probably less than a day of work to implement it, if other's agree that's reasonable I can prepare the changes. |
|
This are the basic changes I'm proposing: adurang@0082f58 |
|
The original patch never should've landed in a state where it broke tests. Unfortunately our test suite is so broken people just break things and leave it because it's already broken. Hopefully things will be more clear from here on out. |
|
Thank you @adurang. I have tested your patch and it gets rid of the linker error. Is that something that can be easily fixed? If not I feel like we should revert for now and take some time to look how to move this forward. |
I think so. Correcting a minor mistake in my previous patch, the interop test is passing with the level zero plugin (the only one I can test unfortunately): For the other plugins, it would be a matter of the migrating the old interfaces. I think I can do that to the same level that was implemented before (i.e., moving code around) but someone might need to fill the parts for the new pieces. Note that even if that test was saying passing before it was kind of lie because the implementation of "use" and "destroy" were doing nothing. I think the tests will probably pass anyway because we're not really checking for any real result but that test wasn't really not a measure of interop working or not working. Shall I try to fix the other plugins? |
Agree that reverting is the right course of action. This functionality can be revisited when the supporting pieces are in place. Let's get OpenMP back to a useable state for now. |
|
We prefer #161429 over this revert. |
|
Closing this as we're going with forward fix. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit 66d1c37.
This patch is the runtime library part of a 2-patch piece of work. The second piece to this is codegen work. For the codegen part, there currently is no clear timeline. However, without the codegen part, this patch breaks interop and does not make sense.
I propose to revert this patch for now and clarify the path forward.