Skip to content

Commit 5dac6b2

Browse files
nickggfacebook-github-bot
authored andcommitted
Revert "Add dynamic buffer support to OCL Backend (#3765)" (#3784)
Summary: This reverts commit bd69664. I had thought that I had gotten the last POCL issue in #3765, but I had not. Reverting to fix the OCL build. Honestly this last issue (AMD/POCL requires sub buffers to aligned) seems to torpedo the whole idea, I can't think of any way to handle Glow TensorViews on the host - which means passing buffer + offset everywhere we pass a buffer below. Essentially this would mean rewriting the whole thing. Very frustrating since that alignment restriction on subBuffers makes no sense, and no other OCL implementation has it. Pull Request resolved: #3784 Differential Revision: D18480248 Pulled By: nickgg fbshipit-source-id: 9b05009ea901a0f477805e6c946faac34d9bc303
1 parent eb48b31 commit 5dac6b2

File tree

8 files changed

+490
-232
lines changed

8 files changed

+490
-232
lines changed

.circleci/build.sh

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ elif [[ "$CIRCLE_JOB" == "PYTORCH" ]]; then
140140
cd build
141141
elif [[ "$CIRCLE_JOB" == "OPENCL" ]]; then
142142
install_pocl
143-
CMAKE_ARGS+=("-DGLOW_WITH_OPENCL=ON" "-DGLOW_OPENCL_ALIGN=128")
143+
CMAKE_ARGS+=("-DGLOW_WITH_OPENCL=ON")
144144
else
145145
CMAKE_ARGS+=("-DCMAKE_BUILD_TYPE=Debug")
146146
if [[ "${CIRCLE_JOB}" == "SHARED" ]]; then

lib/Backends/OpenCL/OpenCL.cpp

Lines changed: 93 additions & 127 deletions
Large diffs are not rendered by default.

lib/Backends/OpenCL/OpenCL.h

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,8 @@ class OpenCLFunction final : public CompiledFunction {
145145
/// Fill the device \p buffer with a given \p value.
146146
/// \param len number of buffer elements to be filled by the \p value.
147147
/// Elements are considered to be of the type described by \p elemKind.
148-
void fillBuffer(cl_mem buffer, uint64_t len, float value, ElemKind elemKind,
148+
void fillBuffer(cl_mem buffer, uint64_t start, uint64_t len, float value,
149+
ElemKind elemKind,
149150
runtime::OpenCLDeviceBindings *devBindings);
150151

151152
/// Execution a convolution instruction which uses NCHW format.
@@ -243,13 +244,10 @@ namespace runtime {
243244
/// device specific information used to run a compiled function on a specific
244245
/// device.
245246
struct OpenCLDeviceBindings : DeviceBindings {
246-
OpenCLDeviceBindings(
247-
cl_mem buffer, cl_command_queue commands, cl_device_id device,
248-
cl_context ctx, cl_program prog,
249-
const std::unordered_map<std::string, cl_mem> &subBuffers)
247+
OpenCLDeviceBindings(cl_mem buffer, cl_command_queue commands,
248+
cl_device_id device, cl_context ctx, cl_program prog)
250249
: DeviceBindings(OCLBackend::getName()), deviceBuffer{buffer},
251-
commandQueue{commands}, deviceId{device}, context{ctx}, program{prog},
252-
weightBuffers(subBuffers) {}
250+
commandQueue{commands}, deviceId{device}, context{ctx}, program{prog} {}
253251

254252
/// CL memory buffer. Currently this contains both mutable and immutable
255253
/// weights, the buffer is allocated once when the network is added.
@@ -273,12 +271,6 @@ struct OpenCLDeviceBindings : DeviceBindings {
273271

274272
/// A list of kernels and their associated events.
275273
std::vector<KernelLaunch> kernelLaunches;
276-
277-
/// Buffers or subBuffers associated with symbols.
278-
std::unordered_map<std::string, cl_mem> weightBuffers;
279-
280-
/// /returns the subBufffer assciated with a Value.
281-
cl_mem getBuffer(glow::Value *v);
282274
};
283275
} // namespace runtime
284276
} // namespace glow

lib/Backends/OpenCL/OpenCLDeviceManager.cpp

Lines changed: 2 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -55,29 +55,7 @@ DeviceManager *createOCLDeviceManager(const DeviceConfig &config) {
5555
return new OpenCLDeviceManager(config);
5656
}
5757

58-
OpenCLBuffer::~OpenCLBuffer() {
59-
for (auto buf : subBuffers_) {
60-
clReleaseMemObject(buf.second);
61-
}
62-
subBuffers_.clear();
63-
64-
clReleaseMemObject(buffer_);
65-
}
66-
67-
/// Add a mapping from a Symbol name to an offset into buffer_;
68-
bool OpenCLBuffer::addSubBuffer(std::string name, size_t offset, size_t size) {
69-
cl_buffer_region region({offset, size});
70-
cl_int err;
71-
auto buf = clCreateSubBuffer(buffer_, CL_MEM_READ_WRITE,
72-
CL_BUFFER_CREATE_TYPE_REGION, &region, &err);
73-
auto res = subBuffers_.emplace(name, buf);
74-
if (!res.second) {
75-
llvm::dbgs() << "OpenCLBuffer: failed to add subBuffer for symbol " << name
76-
<< "\n";
77-
return false;
78-
}
79-
return true;
80-
}
58+
OpenCLBuffer::~OpenCLBuffer() { clReleaseMemObject(buffer_); }
8159
} // namespace runtime
8260
} // namespace glow
8361

@@ -378,15 +356,6 @@ void OpenCLDeviceManager::addNetworkImpl(const Module *module,
378356
clFinish(commands);
379357
}
380358
usedMemoryBytes_ += sizeInBytes;
381-
382-
// Add a sub-buffer for each symbol in the symbol table. OpenCL sub-buffers
383-
// are essentially TensorViews in Glow.
384-
for (auto &pair : bundle.getSymbolTable()) {
385-
bool success = buffer->addSubBuffer(pair.first, pair.second.offset,
386-
pair.second.size);
387-
DCHECK(success);
388-
}
389-
390359
// Compile the CL program.
391360
// Add to the function name lookup map.
392361
// Add shared pointer to the buffer to buffers. This way the buffer will
@@ -407,7 +376,6 @@ void OpenCLDeviceManager::addNetworkImpl(const Module *module,
407376
programs_.emplace(func.first, program);
408377
functions_.emplace(func.first, func.second);
409378
buffers_.emplace(func.first, buffer);
410-
411379
buffer->incrementUsers();
412380

413381
DCHECK_LE(usedMemoryBytes_, maxMemoryBytes_);
@@ -705,7 +673,7 @@ void OpenCLDeviceManager::runFunctionImpl(
705673
auto program = programs_[function];
706674
auto clBindings = glow::make_unique<runtime::OpenCLDeviceBindings>(
707675
buffers_[function]->getBuffer(), queue.backingQueue, deviceId_, context_,
708-
program, buffers_[function]->getSubBuffers());
676+
program);
709677

710678
// Copy inputs to the device.
711679
copyInputsToDevice(func->getRuntimeBundle(), context.get(), clBindings.get());

lib/Backends/OpenCL/OpenCLDeviceManager.h

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -98,9 +98,6 @@ class OpenCLBuffer {
9898
/// The OpenCL buffer being stored.
9999
cl_mem buffer_;
100100

101-
/// Subbuffers for symbols.
102-
std::unordered_map<std::string, cl_mem> subBuffers_;
103-
104101
/// Count of functions using this buffer.
105102
unsigned int users_{0};
106103

@@ -123,14 +120,6 @@ class OpenCLBuffer {
123120

124121
/// Get size of buffer in bytes.
125122
size_t getSize() { return size_; }
126-
127-
/// Return the mapping from Symbol name to subBuffer for this Buffer.
128-
const std::unordered_map<std::string, cl_mem> &getSubBuffers() {
129-
return subBuffers_;
130-
}
131-
132-
/// Add a mapping from a Symbol name to an offset into buffer_;
133-
bool addSubBuffer(std::string name, size_t offset, size_t size);
134123
};
135124

136125
/// A class controlling a single OpenCL device. Many OpenCLFunctions may be

0 commit comments

Comments
 (0)