From 5ba3c05f2abc6a63f9c4ca61fa9014d6fd80203b Mon Sep 17 00:00:00 2001 From: Fraser Cormack Date: Wed, 20 Dec 2023 14:43:03 +0000 Subject: [PATCH] [compiler] Remove the RemoveFencesPass This pass was originally introduced back in LLVM 7 to account for the fact that some of our targets couldn't handle the newly added `fence` instruction. Removing them bypassed the issue. While it is not semantically correct to remove fence instructions from LLVM IR in general, we were relying on the fact that the memory model is under-specified in OpenCL 1.2 and so could assume that fences were redundant. The ENORMOUS WARNING above this scheduling of this pass should be reason enough not to run it. It has not been necessary for several LLVM versions - it is claimed it is only in the host pipeline to ensure it's tested. This is not a good enough reason to keep a dodgy unnecessary pass. There aren't enough good reasons to keep it around unused in the codebase (the pass is trivial to implement by a downstream target if they did require it) so this commit removes the pass. --- doc/modules/compiler/utils.rst | 7 --- .../source/base_module_pass_machinery.cpp | 1 - .../base/source/base_module_pass_registry.def | 1 - .../targets/host/source/HostPassMachinery.cpp | 15 ------- modules/compiler/utils/CMakeLists.txt | 2 - .../compiler/utils/remove_fences_pass.h | 44 ------------------- .../utils/source/remove_fences_pass.cpp | 37 ---------------- 7 files changed, 107 deletions(-) delete mode 100644 modules/compiler/utils/include/compiler/utils/remove_fences_pass.h delete mode 100644 modules/compiler/utils/source/remove_fences_pass.cpp diff --git a/doc/modules/compiler/utils.rst b/doc/modules/compiler/utils.rst index 465aa4219..8620ad1ba 100644 --- a/doc/modules/compiler/utils.rst +++ b/doc/modules/compiler/utils.rst @@ -1139,13 +1139,6 @@ Removing this information is useful for debugging since the backend is less likely to optimize away variables in the stack no longer used, as a result this pass should only be run on debug builds of the module. -RemoveFencesPass ----------------- - -Removing memory fences can result in invalid code or incorrect behaviour in -general. This pass is a workaround for backends that do not yet support memory -fences. - RemoveExceptionsPass -------------------- diff --git a/modules/compiler/source/base/source/base_module_pass_machinery.cpp b/modules/compiler/source/base/source/base_module_pass_machinery.cpp index dede768e1..457ff3680 100644 --- a/modules/compiler/source/base/source/base_module_pass_machinery.cpp +++ b/modules/compiler/source/base/source/base_module_pass_machinery.cpp @@ -49,7 +49,6 @@ #include #include #include -#include #include #include #include diff --git a/modules/compiler/source/base/source/base_module_pass_registry.def b/modules/compiler/source/base/source/base_module_pass_registry.def index 659f55c81..7da945152 100644 --- a/modules/compiler/source/base/source/base_module_pass_registry.def +++ b/modules/compiler/source/base/source/base_module_pass_registry.def @@ -177,7 +177,6 @@ FUNCTION_PASS("software-div", compiler::SoftwareDivisionPass()) FUNCTION_PASS("replace-addrspace-fns", compiler::utils::ReplaceAddressSpaceQualifierFunctionsPass()) FUNCTION_PASS("remove-lifetime", compiler::utils::RemoveLifetimeIntrinsicsPass()) FUNCTION_PASS("remove-exceptions", compiler::utils::RemoveExceptionsPass()) -FUNCTION_PASS("remove-fences", compiler::utils::RemoveFencesPass()) FUNCTION_PASS("replace-mem-intrins", compiler::utils::ReplaceMemIntrinsicsPass()) FUNCTION_PASS("print", compiler::utils::GenericMetadataPrinterPass(llvm::dbgs())) FUNCTION_PASS("print", compiler::utils::VectorizeMetadataPrinterPass(llvm::dbgs())) diff --git a/modules/compiler/targets/host/source/HostPassMachinery.cpp b/modules/compiler/targets/host/source/HostPassMachinery.cpp index 02eeaecb9..773f3d406 100644 --- a/modules/compiler/targets/host/source/HostPassMachinery.cpp +++ b/modules/compiler/targets/host/source/HostPassMachinery.cpp @@ -32,7 +32,6 @@ #include #include #include -#include #include #include #include @@ -311,20 +310,6 @@ llvm::ModulePassManager HostPassMachinery::getKernelFinalizationPasses( compiler::utils::RemoveLifetimeIntrinsicsPass())); } - // ENORMOUS WARNING: - // Removing memory fences can result in invalid code or incorrect behaviour in - // general. This pass is a workaround for backends that do not yet support - // memory fences. This is not required for any of the LLVM backends used by - // host, but the pass is used here to ensure that it is tested. - // The memory model on OpenCL 1.2 is so underspecified that we can get away - // with removing fences. In OpenCL 3.0 the memory model is better defined, and - // just removing fences could result in incorrect behavior for valid 3.0 - // OpenCL applications. - if (options.standard != compiler::Standard::OpenCLC30) { - PM.addPass(llvm::createModuleToFunctionPassAdaptor( - compiler::utils::RemoveFencesPass())); - } - PM.addPass(compiler::utils::ComputeLocalMemoryUsagePass()); PM.addPass(compiler::utils::AddMetadataPass< diff --git a/modules/compiler/utils/CMakeLists.txt b/modules/compiler/utils/CMakeLists.txt index 1666c5ec6..e238dbed5 100644 --- a/modules/compiler/utils/CMakeLists.txt +++ b/modules/compiler/utils/CMakeLists.txt @@ -52,7 +52,6 @@ add_ca_library(compiler-utils STATIC ${CMAKE_CURRENT_SOURCE_DIR}/include/compiler/utils/prepare_barriers_pass.h ${CMAKE_CURRENT_SOURCE_DIR}/include/compiler/utils/reduce_to_function_pass.h ${CMAKE_CURRENT_SOURCE_DIR}/include/compiler/utils/remove_exceptions_pass.h - ${CMAKE_CURRENT_SOURCE_DIR}/include/compiler/utils/remove_fences_pass.h ${CMAKE_CURRENT_SOURCE_DIR}/include/compiler/utils/remove_lifetime_intrinsics_pass.h ${CMAKE_CURRENT_SOURCE_DIR}/include/compiler/utils/rename_builtins_pass.h ${CMAKE_CURRENT_SOURCE_DIR}/include/compiler/utils/replace_address_space_qualifier_functions_pass.h @@ -104,7 +103,6 @@ add_ca_library(compiler-utils STATIC ${CMAKE_CURRENT_SOURCE_DIR}/source/prepare_barriers_pass.cpp ${CMAKE_CURRENT_SOURCE_DIR}/source/reduce_to_function_pass.cpp ${CMAKE_CURRENT_SOURCE_DIR}/source/remove_exceptions_pass.cpp - ${CMAKE_CURRENT_SOURCE_DIR}/source/remove_fences_pass.cpp ${CMAKE_CURRENT_SOURCE_DIR}/source/remove_lifetime_intrinsics_pass.cpp ${CMAKE_CURRENT_SOURCE_DIR}/source/rename_builtins_pass.cpp ${CMAKE_CURRENT_SOURCE_DIR}/source/replace_address_space_qualifier_functions_pass.cpp diff --git a/modules/compiler/utils/include/compiler/utils/remove_fences_pass.h b/modules/compiler/utils/include/compiler/utils/remove_fences_pass.h deleted file mode 100644 index 24a38c7e2..000000000 --- a/modules/compiler/utils/include/compiler/utils/remove_fences_pass.h +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright (C) Codeplay Software Limited -// -// Licensed under the Apache License, Version 2.0 (the "License") with LLVM -// Exceptions; you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -// License for the specific language governing permissions and limitations -// under the License. -// -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - -/// @file -/// -/// Remove fences pass - -#ifndef COMPILER_UTILS_REMOVE_FENCES_PASS_H_INCLUDED -#define COMPILER_UTILS_REMOVE_FENCES_PASS_H_INCLUDED - -#include - -namespace compiler { -namespace utils { - -/// @brief A pass that removes all memory fence instructions. -/// -/// ENORMOUS WARNING: -/// Removing memory fences can result in invalid code or incorrect behaviour in -/// general. This pass is a workaround for backends that do not yet support -/// memory fences. -class RemoveFencesPass final : public llvm::PassInfoMixin { - public: - llvm::PreservedAnalyses run(llvm::Function &, - llvm::FunctionAnalysisManager &); -}; - -} // namespace utils -} // namespace compiler - -#endif // COMPILER_UTILS_REMOVE_FENCES_PASS_H_INCLUDED diff --git a/modules/compiler/utils/source/remove_fences_pass.cpp b/modules/compiler/utils/source/remove_fences_pass.cpp deleted file mode 100644 index 697f03741..000000000 --- a/modules/compiler/utils/source/remove_fences_pass.cpp +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright (C) Codeplay Software Limited -// -// Licensed under the Apache License, Version 2.0 (the "License") with LLVM -// Exceptions; you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// https://github.com/codeplaysoftware/oneapi-construction-kit/blob/main/LICENSE.txt -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -// License for the specific language governing permissions and limitations -// under the License. -// -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - -#include -#include - -using namespace llvm; - -PreservedAnalyses compiler::utils::RemoveFencesPass::run( - Function &F, FunctionAnalysisManager &) { - bool Changed = false; - - for (BasicBlock &BB : F) { - for (auto InstI = BB.begin(); InstI != BB.end();) { - auto &Inst = *InstI++; - if (isa(Inst)) { - Inst.eraseFromParent(); - Changed = true; - } - } - } - - return Changed ? PreservedAnalyses::none() : PreservedAnalyses::all(); -}