[SandboxIR] Delete SandboxIR/SandboxVectorizer #166417
Conversation
These components are still experimental, have not recieved commit traffic beyond basic maintenance since May, and it was decided that the people at Google working on the project should focus on other priorities. Given the lack of investment, delete the code for now. If someone wants to revive this, they can revert this patch. For now, delete it so that upstream does not have to bear the maintenance cost.
|
@llvm/pr-subscribers-vectorizers @llvm/pr-subscribers-llvm-transforms Author: Aiden Grossman (boomanaiden154) ChangesThese components are still experimental, have not recieved commit traffic beyond basic maintenance since May, and it was decided that the people at Google working on the project should focus on other priorities. Given the lack of investment, delete the code for now. If someone wants to revive this, they can revert this patch. For now, delete it so that upstream does not have to bear the maintenance cost. Patch is 1.27 MiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/166417.diff 128 Files Affected:
diff --git a/llvm/Maintainers.md b/llvm/Maintainers.md
index 1eba955f9d6ed..d6d69aaf81532 100644
--- a/llvm/Maintainers.md
+++ b/llvm/Maintainers.md
@@ -101,13 +101,6 @@ asbirlea@google.com (email), [alinas](https://github.com/alinas) (GitHub)
Madhur Amilkanthwar \
madhura@nvidia.com (email), [madhur13490](https://github.com/madhur13490) (GitHub)
-#### SandboxVectorizer
-
-Vasileios Porpodas \
-vporpodas@google.com (email), [vporpo](https://github.com/vporpo) (GitHub) \
-Jorge Gorbe Moya \
-jgorbe@google.com (email), [slackito](https://github.com/slackito) (GitHub)
-
#### ScalarEvolution, IndVarSimplify
Philip Reames \
@@ -397,13 +390,6 @@ geek4civic@gmail.com (email), [chapuni](https://github.com/chapuni) (GitHub)
Lang Hames \
lhames@gmail.com (email), [lhames](https://github.com/lhames) (GitHub)
-#### SandboxIR
-
-Vasileios Porpodas \
-vporpodas@google.com (email), [vporpo](https://github.com/vporpo) (GitHub) \
-Jorge Gorbe Moya \
-jgorbe@google.com (email), [slackito](https://github.com/slackito) (GitHub)
-
#### TableGen
Rahul Joshi \
diff --git a/llvm/benchmarks/CMakeLists.txt b/llvm/benchmarks/CMakeLists.txt
index e411ed4326a36..40a11dc4ed16f 100644
--- a/llvm/benchmarks/CMakeLists.txt
+++ b/llvm/benchmarks/CMakeLists.txt
@@ -1,7 +1,6 @@
set(LLVM_LINK_COMPONENTS
AsmParser
Core
- SandboxIR
Support)
add_benchmark(DummyYAML DummyYAML.cpp PARTIAL_SOURCES_INTENDED)
@@ -9,7 +8,6 @@ add_benchmark(xxhash xxhash.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(GetIntrinsicForClangBuiltin GetIntrinsicForClangBuiltin.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(FormatVariadicBM FormatVariadicBM.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(GetIntrinsicInfoTableEntriesBM GetIntrinsicInfoTableEntriesBM.cpp PARTIAL_SOURCES_INTENDED)
-add_benchmark(SandboxIRBench SandboxIRBench.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(MustacheBench Mustache.cpp PARTIAL_SOURCES_INTENDED)
add_benchmark(SpecialCaseListBM SpecialCaseListBM.cpp PARTIAL_SOURCES_INTENDED)
diff --git a/llvm/benchmarks/SandboxIRBench.cpp b/llvm/benchmarks/SandboxIRBench.cpp
deleted file mode 100644
index 45f352697868b..0000000000000
--- a/llvm/benchmarks/SandboxIRBench.cpp
+++ /dev/null
@@ -1,243 +0,0 @@
-//===- SandboxIRBench.cpp -------------------------------------------------===//
-//
-// 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
-//
-//===----------------------------------------------------------------------===//
-//
-// These tests measure the performance of some core SandboxIR functions and
-// compare them against LLVM IR.
-//
-//===----------------------------------------------------------------------===//
-
-#include "benchmark/benchmark.h"
-#include "llvm/AsmParser/Parser.h"
-#include "llvm/IR/BasicBlock.h"
-#include "llvm/IR/DataLayout.h"
-#include "llvm/IR/Function.h"
-#include "llvm/IR/Instruction.h"
-#include "llvm/IR/Module.h"
-#include "llvm/SandboxIR/Function.h"
-#include "llvm/SandboxIR/Instruction.h"
-#include "llvm/SandboxIR/Module.h"
-#include "llvm/Support/SourceMgr.h"
-#include <memory>
-#include <sstream>
-
-using namespace llvm;
-
-static std::unique_ptr<Module> parseIR(LLVMContext &C, const char *IR) {
- SMDiagnostic Err;
- std::unique_ptr<Module> M = parseAssemblyString(IR, Err, C);
- if (!M)
- Err.print("SandboxIRBench", errs());
- return M;
-}
-
-enum class IR {
- LLVM, ///> LLVM IR
- SBoxNoTracking, ///> Sandbox IR with tracking disabled
- SBoxTracking, ///> Sandbox IR with tracking enabled
-};
-// Traits to get llvm::BasicBlock/sandboxir::BasicBlock from IR::LLVM/IR::SBox.
-template <IR IRTy> struct TypeSelect {};
-template <> struct TypeSelect<IR::LLVM> {
- using BasicBlock = llvm::BasicBlock;
-};
-template <> struct TypeSelect<IR::SBoxNoTracking> {
- using BasicBlock = sandboxir::BasicBlock;
-};
-template <> struct TypeSelect<IR::SBoxTracking> {
- using BasicBlock = sandboxir::BasicBlock;
-};
-
-template <IR IRTy>
-static typename TypeSelect<IRTy>::BasicBlock *
-genIR(std::unique_ptr<llvm::Module> &LLVMM, LLVMContext &LLVMCtx,
- sandboxir::Context &Ctx,
- std::function<std::string(unsigned)> GenerateIRStr,
- unsigned NumInstrs = 0u) {
- std::string IRStr = GenerateIRStr(NumInstrs);
- LLVMM = parseIR(LLVMCtx, IRStr.c_str());
- llvm::Function *LLVMF = &*LLVMM->getFunction("foo");
- llvm::BasicBlock *LLVMBB = &*LLVMF->begin();
-
- sandboxir::Function *F = Ctx.createFunction(LLVMF);
- sandboxir::BasicBlock *BB = &*F->begin();
- // Start tracking if we are testing with tracking enabled.
- if constexpr (IRTy == IR::SBoxTracking)
- Ctx.save();
-
- if constexpr (IRTy == IR::LLVM)
- return LLVMBB;
- else
- return BB;
-}
-
-template <IR IRTy> static void finalize(sandboxir::Context &Ctx) {
- // Accept changes if we are tracking.
- if constexpr (IRTy == IR::SBoxTracking)
- Ctx.accept();
-}
-
-static std::string generateBBWalkIR(unsigned Size) {
- std::stringstream SS;
- SS << "define void @foo(i32 %v1, i32 %v2) {\n";
- for (auto Cnt : seq<unsigned>(0, Size))
- SS << " %add" << Cnt << " = add i32 %v1, %v2\n";
- SS << "ret void";
- SS << "}";
- return SS.str();
-}
-
-template <IR IRTy> static void SBoxIRCreation(benchmark::State &State) {
- static_assert(IRTy != IR::LLVM, "Expected SBoxTracking or SBoxNoTracking");
- LLVMContext LLVMCtx;
- unsigned NumInstrs = State.range(0);
- std::unique_ptr<llvm::Module> LLVMM;
- std::string IRStr = generateBBWalkIR(NumInstrs);
- LLVMM = parseIR(LLVMCtx, IRStr.c_str());
- llvm::Function *LLVMF = &*LLVMM->getFunction("foo");
-
- for (auto _ : State) {
- State.PauseTiming();
- sandboxir::Context Ctx(LLVMCtx);
- if constexpr (IRTy == IR::SBoxTracking)
- Ctx.save();
- State.ResumeTiming();
-
- sandboxir::Function *F = Ctx.createFunction(LLVMF);
- benchmark::DoNotOptimize(F);
- State.PauseTiming();
- if constexpr (IRTy == IR::SBoxTracking)
- Ctx.accept();
- State.ResumeTiming();
- }
-}
-
-template <IR IRTy> static void BBWalk(benchmark::State &State) {
- LLVMContext LLVMCtx;
- sandboxir::Context Ctx(LLVMCtx);
- unsigned NumInstrs = State.range(0);
- std::unique_ptr<llvm::Module> LLVMM;
- auto *BB = genIR<IRTy>(LLVMM, LLVMCtx, Ctx, generateBBWalkIR, NumInstrs);
- for (auto _ : State) {
- // Walk LLVM Instructions.
- for (auto &I : *BB)
- benchmark::DoNotOptimize(I);
- }
-}
-
-static std::string generateGetTypeIR(unsigned Size) {
- return R"IR(
-define void @foo(i32 %v1, i32 %v2) {
- %add = add i32 %v1, %v2
- ret void
-}
-)IR";
-}
-
-template <IR IRTy> static void GetType(benchmark::State &State) {
- LLVMContext LLVMCtx;
- sandboxir::Context Ctx(LLVMCtx);
- std::unique_ptr<llvm::Module> LLVMM;
- auto *BB = genIR<IRTy>(LLVMM, LLVMCtx, Ctx, generateGetTypeIR);
- auto *I = &*BB->begin();
- for (auto _ : State)
- benchmark::DoNotOptimize(I->getType());
-}
-
-static std::string generateRAUWIR(unsigned Size) {
- std::stringstream SS;
- SS << "define void @foo(i32 %v1, i32 %v2) {\n";
- SS << " %def1 = add i32 %v1, %v2\n";
- SS << " %def2 = add i32 %v1, %v2\n";
- for (auto Cnt : seq<unsigned>(0, Size))
- SS << " %add" << Cnt << " = add i32 %def1, %def1\n";
- SS << "ret void";
- SS << "}";
- return SS.str();
-}
-
-template <IR IRTy> static void RAUW(benchmark::State &State) {
- LLVMContext LLVMCtx;
- sandboxir::Context Ctx(LLVMCtx);
- std::unique_ptr<llvm::Module> LLVMM;
- unsigned NumInstrs = State.range(0);
- auto *BB = genIR<IRTy>(LLVMM, LLVMCtx, Ctx, generateRAUWIR, NumInstrs);
- auto It = BB->begin();
- auto *Def1 = &*It++;
- auto *Def2 = &*It++;
- for (auto _ : State) {
- Def1->replaceAllUsesWith(Def2);
- Def2->replaceAllUsesWith(Def1);
- }
- finalize<IRTy>(Ctx);
-}
-
-static std::string generateRUOWIR(unsigned NumOperands) {
- std::stringstream SS;
- auto GenOps = [&SS, NumOperands]() {
- for (auto Cnt : seq<unsigned>(0, NumOperands)) {
- SS << "i8 %arg" << Cnt;
- bool IsLast = Cnt + 1 == NumOperands;
- if (!IsLast)
- SS << ", ";
- }
- };
-
- SS << "define void @foo(";
- GenOps();
- SS << ") {\n";
-
- SS << " call void @foo(";
- GenOps();
- SS << ")\n";
- SS << "ret void";
- SS << "}";
- return SS.str();
-}
-
-template <IR IRTy> static void RUOW(benchmark::State &State) {
- LLVMContext LLVMCtx;
- sandboxir::Context Ctx(LLVMCtx);
- std::unique_ptr<llvm::Module> LLVMM;
- unsigned NumOperands = State.range(0);
- auto *BB = genIR<IRTy>(LLVMM, LLVMCtx, Ctx, generateRUOWIR, NumOperands);
-
- auto It = BB->begin();
- auto *F = BB->getParent();
- auto *Arg0 = F->getArg(0);
- auto *Arg1 = F->getArg(1);
- auto *Call = &*It++;
- for (auto _ : State)
- Call->replaceUsesOfWith(Arg0, Arg1);
- finalize<IRTy>(Ctx);
-}
-
-// Measure the time it takes to create Sandbox IR without/with tracking.
-BENCHMARK(SBoxIRCreation<IR::SBoxNoTracking>)
- ->Args({10})
- ->Args({100})
- ->Args({1000});
-BENCHMARK(SBoxIRCreation<IR::SBoxTracking>)
- ->Args({10})
- ->Args({100})
- ->Args({1000});
-
-BENCHMARK(GetType<IR::LLVM>);
-BENCHMARK(GetType<IR::SBoxNoTracking>);
-
-BENCHMARK(BBWalk<IR::LLVM>)->Args({1024});
-BENCHMARK(BBWalk<IR::SBoxTracking>)->Args({1024});
-
-BENCHMARK(RAUW<IR::LLVM>)->Args({512});
-BENCHMARK(RAUW<IR::SBoxNoTracking>)->Args({512});
-BENCHMARK(RAUW<IR::SBoxTracking>)->Args({512});
-
-BENCHMARK(RUOW<IR::LLVM>)->Args({4096});
-BENCHMARK(RUOW<IR::SBoxNoTracking>)->Args({4096});
-BENCHMARK(RUOW<IR::SBoxTracking>)->Args({4096});
-
-BENCHMARK_MAIN();
diff --git a/llvm/docs/SandboxIR.md b/llvm/docs/SandboxIR.md
deleted file mode 100644
index d2b612ba95ef1..0000000000000
--- a/llvm/docs/SandboxIR.md
+++ /dev/null
@@ -1,109 +0,0 @@
-# Sandbox IR: A transactional layer over LLVM IR
-
-Sandbox IR is an IR layer on top of LLVM IR that allows you to save/restore its state.
-
-# Quick Start Notes
-
-Within your LLVM pass:
-
-``` C++
-// 1. Include the necessary Sandbox IR header files.
-#include "llvm/SandboxIR/Context.h
-#include "llvm/SandboxIR/Function.h
-
-// 2. Create a sandboxir::Context using LLVMContext `LLVMCtx`.
-sandboxir::Context Ctx(LLVMCtx);
-
-// 3. Create a sandboxir::Function using LLVM IR Function `LLVMF`.
-auto *F = Ctx.createFunction(LLVMF);
-
-// ... Use Sandbox IR in `F` as usual, e.g., iterating, modifying it etc. ...
-
-// 4. Save state when needed.
-Ctx.save();
-
-// ... Modify Sandbox IR ...
-
-// 5. Revert to the saved state.
-Ctx.revert();
-```
-
-Make sure you link against `SandboxIR` in `CMakeLists.txt`:
-
-```
-LINK_COMPONENTS
-...
-SandboxIR
-...
-```
-
-# API
-The Sandbox IR API is designed to feel like LLVM, replicating many common API classes and functions to mirror the LLVM API.
-The class hierarchy is similar (but in the `llvm::sandboxir` namespace).
-For example here is a small part of it:
-```
-namespace sandboxir {
- Value
- / \
- User BasicBlock ...
- / \
- Instruction Constant
- /
- ...
-}
-```
-
-# Design
-
-## Sandbox IR Value <-> LLVM IR Value Mapping
-Each LLVM IR Value maps to a single Sandbox IR Value.
-The reverse is also true in most cases, except for Sandbox IR Instructions that map to more than one LLVM IR Instruction.
-Such instructions can be defined in extensions of the base Sandbox IR.
-
-- Forward mapping: Sandbox IR Value -> LLVM IR Value
-Each Sandbox IR Value contains an `llvm::Value *Val` member variable that points to the corresponding LLVM IR Value.
-
-- Reverse mapping: LLVM IR Value -> Sandbox IR Value
-This mapping is stored in `sandboxir::Context::LLVMValueToValue`.
-
-For example `sandboxir::User::getOperand(OpIdx)` for a `sandboxir::User *U` works as follows:
-- First we find the LLVM User: `llvm::User *LLVMU = U->Val`.
-- Next we get the LLVM Value operand: `llvm::Value *LLVMOp = LLVMU->getOperand(OpIdx)`
-- Finally we get the Sandbox IR operand that corresponds to `LLVMOp` by querying the map in the Sandbox IR context: `retrun Ctx.getValue(LLVMOp)`.
-
-## Sandbox IR is Write-Through
-Sandbox IR is designed to rely on LLVM IR for its state.
-So any change made to Sandbox IR objects directly updates the corresponding LLVM IR.
-
-This has the following benefits:
-- It minimizes the replication of state, and
-- It makes sure that Sandbox IR and LLVM IR are always in sync, which helps avoid bugs and removes the need for a lowering step.
-- No need for serialization/de-serialization infrastructure as we can rely on LLVM IR for it.
-- One can pass actual `llvm::Instruction`s to cost modeling APIs.
-
-Sandbox IR API functions that modify the IR state call the corresponding LLVM IR function that modifies the LLVM IR's state.
-For example, for `sandboxir::User::setOperand(OpIdx, sandboxir::Value *Op)`:
-- We get the corresponding LLVM User: `llvm::User *LLVMU = cast<llvm::User>(Val)`
-- Next we get the corresponding LLVM Operand: `llvm::Value *LLVMOp = Op->Val`
-- Finally we modify `LLVMU`'s operand: `LLVMU->setOperand(OpIdx, LLVMOp)
-
-## IR Change Tracking
-Sandbox IR's state can be saved and restored.
-This is done with the help of the tracker component that is tightly coupled to the public Sandbox IR API functions.
-Please note that nested saves/restores are currently not supported.
-
-To save the state and enable tracking the user needs to call `sandboxir::Context::save()`.
-From this point on any change made to the Sandbox IR state will automatically create a change object and register it with the tracker, without any intervention from the user.
-The changes are accumulated in a vector within the tracker.
-
-To rollback to the saved state the user needs to call `sandboxir::Context::revert()`.
-Reverting back to the saved state is a matter of going over all the accumulated changes in reverse and undoing each individual change.
-
-To accept the changes made to the IR the user needs to call `sandboxir::Context::accept()`.
-Internally this will go through the changes and run any finalization required.
-
-Please note that after a call to `revert()` or `accept()` tracking will stop.
-To start tracking again, the user needs to call `save()`.
-
-# Users of Sandbox IR
-- [The Sandbox Vectorizer](project:SandboxVectorizer.md)
diff --git a/llvm/docs/SandboxVectorizer.md b/llvm/docs/SandboxVectorizer.md
deleted file mode 100644
index 2b2401a4bf92b..0000000000000
--- a/llvm/docs/SandboxVectorizer.md
+++ /dev/null
@@ -1,280 +0,0 @@
-# The Sandbox Vectorizer
-
-```{contents}
-:depth: 4
-```
-
-The Sandbox Vectorizer is a framework for building modular vectorization pipelines on top of [Sandbox IR](#SandboxIR) transactional IR, with a focus on ease of development and testing.
-The default pipeline currently implements a simple SLP-style bottom-up vectorization pipeline.
-
-The transactional IR helps in several ways:
-- It enables a modular design where:
- - Each vectorization transformation/optimization can be implemented as a separate internal pass that uses actual IR as its input and output.
- - You can still make end-to-end profitability decisions (i.e., across multiple internal passes), even when the transformations are implemented as separate internal passes.
- - Each transformation/optimization internal pass can be tested in isolation with lit-tests, as opposed to end-to-end tests.
-- It enables a simpler design by enabling each internal pass commit its state to the IR itself rather than updating helper data-structures that live across the pipeline.
-- Its extensive callback interface helps remove the burden of manually maintaining the vectorizer's components while the IR is being modified.
-
-## Usage
-
-The Sandbox Vectorizer is currently under development and is not enabled by default.
-So in order to use it you have to explicitly run the pass with `opt` like so:
-
-```shell
-$ opt -p=sandbox-vectorizer file.ll
-```
-
-## Internal Pass Pipeline
-
-The Sandbox Vectorizer is designed to be modular and as such it has its own internal pass-pipeline that operates on Sandbox IR.
-Each vectorization phase is implemented as a separate internal pass that runs by the Sandbox Vectorizer's internal pass manager.
-The Sandbox Vectorizer pass itself is an LLVM Function pass.
-
-The following figure shows the basic structure of the Sandbox Vectorizer LLVM Function pass.
-The first component is the conversion of `LLVM IR to Sandbox IR` which converts the LLVM Function to a `sandboxir::Function`.
-From this point on the pass operates on Sandbox IR.
-The main entry point to the internal pass pipeline is the `Sandbox IR Function Pass Manger`, which runs all registered function passes.
-The following figure lists only a single Sandbox IR function pass, the `Seed Collection Pass` which goes over the instructions in the function and collects vectorization candidates, like Stores to consecutive memory addresses, and forms a [Region](#region).
-The `Seed Collection Pass` itself contains its own Region pass pipeline, which in the following example contains a `Transaction Save` pass, a `Bottom-Up Vectorization` pass, a `Pack Reuse` pass and a `Transaction Accept/Revert` pass.
-
-```
-┌────────────────────────────────── Sandbox Vectorizer LLVM Function Pass ─────────────────────────────┐
-│ │
-│ ┌───────┐ ┌────────────────────────── sandboxir::Function Pass Manager ────────────────────────────┐ │
-│ │ │ │ │ │
-│ │ │ │ ┌────────────────────────────── Seed Collection Pass ──────────────────────────────┐ │ │
-│ │ │ │ │ │ │ │
-│ │ │ │ │ ┌───────┐ For ┌─────────────── sanboxir::Region Pass Manager ───────────────┐ │ │ │
-│ │LLVM IR│ │ │ │Collect│ each │ ┌───────────┐ ┌────────────────┐ ┌───────┐ ┌──────────────┐ │ │ │ │
-│ │ to │ │ │ │ Seeds │ Region │ │Transaction│ │ Bottom─Up │ │ Pack │ │ Transaction │ │ │ │ │
-│ │Sandbox│ │ │ │Create │ ─────> │ │ Save │ │ Vectorization │ │ Reuse │ │Accept/Revert │ │ │ │ │
-│ │ IR │ │ │ │Regions│ │ └───────────┘ └────────────────┘ └───────┘ └──────────────┘ │ │ │ │
-│ │ │ │ │ └───────┘ └─────────────────────────────────────────────────────────────┘ │ │ │
-│ │ │ │ │ │...│ │
-│ │ │ │ └──────────────────────────────────────────────────────────────────────────────────┘ │ │
-│ │ │ │ │ │
-│ └───────┘ └────────────────────────────────────────────────────────────────────────────────────────┘ │
-│ │
-└──────────────────────────────────────────────────────────────────────────────────────────────────────┘
-```
-
-You can specify your own custom pipeline with the `-sbvec-passes=` argument to `opt`.
-The pipeline shown above is equivalent to this:
-
-```shell
-$ opt -p=sandbox-vectorizer -sbvec-passes='seed-collection<tr-save,bottom-up-vec,pack-reuse,tr-accept>' file.ll
-```
-
-If the user does not define a pipeline, the Sandbox Vectorizer will run its default pass-pipeline, which is set in the constructor of the `SandboxVectorizerPass`.
-
-## Sandbox Vectorizer Passes
-
-The passes in the vectorization pipeline can be found in `Transforms/Vectorize/SandboxVectorizer/Passes` and they are registered in `lib/Transforms/Vectorize/SandboxVectorizer/Passes/PassRegistry.def`.
-
-There are two types of passes: [Transformation Passes](#transformation-passes) that do the actual vectorization-related transformations and optimizations, and [Helper Passes](#helper-passes) that are helping with things like managing the IR transactions, and test-specific things like building regions.
-
-### Transformation Passes
-
-| **Pass Name** | **File Name** | **Type** | **Description** |
-|---------------------------|-----------------------------|----------|---------------------------------------------------------|
-| `seed-collection` | SeedCollection.h | Function | Col...
[truncated]
|
|
Hi Aiden, It is true that the main authors and maintainers of this project no longer work at Google, and that activity has slowed down since about a year ago when I stopped working on it full time. But as you probably already know, this doesn't mean that the project is not being worked on, or maintained.
Both Sandbox IR and the Sandbox Vectorizer are self-contained. The only changes that would need maintenance are the ones that modify the core IR. I can see just one such change, the addition of the ptrtoaddr instruction in LLVM IR: 3a4b351. Such changes are quite rare and usually need just a few straightforward changes in Sandbox IR. |
Ack. It's still only used by the Sandbox Vectorizer though, and if we're looking at moving that out of tree, I think removing Sandbox IR at the same time also makes sense.
Yes, I saw that patch. But there haven't been any patches that have landed since June, so it seems like development has stalled. @slackito, the only reviewer pinged on that PR is also no longer working at Google and I do not think he plans to carry on any review around Sandbox IR/Vectorizer.
Sure, these changes are rare and do not need much work. They still require effort though. There are also a lot of more general code style and similar cleanups that also get performed across the repo, including inside the Sandbox directories. I also don't think stability/self-containment are a good enough reason to keep it in tree. Development has stalled, or at least slowed significantly given there is no funding behind it, it does not seem like there are any plans/interest in significantly changing this, and there are also no users of this. There is plenty of stable code in LLVM that sticks around, but it has users that justify its existence. There are also test/CI costs around running the tests and building the code. They are marginal, but still present. |
The decision why this project was developed in tree in the first place was discussed in the RFC. I am not sure why this is being brought up again.
How did you come to this conclusion? Just because development is slow, it doesn't mean that there are no plans or interest in the project.
There are plenty of components in LLVM without patches in the last couple of months. In the Sandbox Vectorizer's case there are at least pending patches in the pipeline. The only reason they haven't been checked in yet, is because I am trying to stick to the LLVM community's code review standards and get them thoroughly reviewed upstream by developers who are familiar with the codebase before landing. Just because a reviewer might be temporarily unavailable, it doesn't suggest that they won't be available in the future. Regarding the maintenance cost, I would argue that it hasn't been a burden to anyone yet. Nobody so far has raised any actual concerns regarding code maintenance, at least nobody has reached out to me. As the active maintainer, I am always available to help with any issues that show up. I feel it's a bit too early to suggest moving the project out of tree. As far as I am aware the components have not been a maintenance burden, so I don't see the urgency. The Sandbox Vectorizer already provides a really nice toolset of modular components that can be used to build modular vectorization pipelines. |
I'm familiar with the conversation there. I'm bringing this up again because the context surrounding the project has changed. At that point in time, the project was well funded and a lot of work was happening. Now the work is unfunded with the vectorizer not yet usable (at least in isolation) for production workloads.
I guess plans/interest was not the right term. It does seem like you have plans and interest in completing it, but maybe a lack of time/resources/reviewers to see the project through quickly.
Having talked to all of the people who worked on this project at Google, it does not sound like there are any plans to continue reviewing these patches. I think if you want to make progress you are going to need to find new reviewers.
Ack. It's not significant yet, but it is essentially dead code, which we typically try to remove. I honestly think focusing on development out of tree would be better for this project. It seems like reviewer bandwidth is a large issue currently given the changes in backing. Developing out of tree would let you build out the rest of the vectorizer until it can compete with the existing SLPVectorizer implementation, which I think would help people justify spending their time reviewing the rest of the patches. And I think that can happen regardless of if this patch lands or not. If we land this patch, we remove the small yet real maintenance burden for now, and reverting should be easy given this code has already been reviewed. I know this is mostly a rehash of some of the conversations in the original RFC, but I think the lack of funding to finish the implementation changes things significantly. If you still want to keep this in tree despite the reviewer bandwidth issue, can we come up with a soft timeline? Eg if no one is using/enabling this infrastructure anywhere, or there have been significant changes in reviewer bandwidth or funding in 12 months then we move it out of tree? |
|
The goal of the project was never to compete with the SLP Vectorizer, this is clearly stated in the RFC and the discussions that followed. The Sandbox Vectorizer would be yet another vectorizer component alongside VPlan, SLPVectorizer and the LoadStoreVectorizer with focus on modularity, stability and ease of testing. To recap the Sandbox Vectorizer project's goal has been twofold:
The whole project reached stable state just a few months ago, see my RFC update and there are still components and features under development. So I hardly see this as "dead code". |
Just to give a small data point here: I recently tried to improve block successor iteration, which requires changes to IR data structures (e.g. here). Obviously, this also needs SandboxIR changes, which does cost time to read into, get familiar with how it works, etc., and as I'm not familiar with SandboxIR at all, nothing is really straightforward for me. So there is a very real cost from having this in-tree and built by default. IR changes are already very time consuming (and allocating time for upstream LLVM work is already hard for many, certainly for me), so I would appreciate if I at least could say "fixing SandboxIR is someone else's problem" (similar to some back-ends). |
That's not always true. Sandbox IR is by design not tightly coupled to LLVM IR:
So in most cases I would argue that "fixing SandboxIR is someone else's problem" is actually true by design. I would be interested to know more about how Sandbox IR "broke you" in the patch you shared. It looks like your patch is mostly changing the internals of the IR which shouldn't cause an issue with Sandbox IR. |
|
I'm happy to keep contributing with the occasional code review, but I'm not a vectorization expert, so unless the Sandbox Vectorizer catches the attention of other developers in the community I expect @vporpo to keep being the sole developer. I don't know if that's enough activity by the standards of the LLVM community to keep the project in-tree. I do sympathize with the desire to avoid the ongoing maintenance costs, though. If SandboxIR breaks often for people contributing to other parts of LLVM, that'd be a big argument in favor of the removal IMHO. |
It changes the operand meaning and requires additional storage for the switch constants (which are no longer operands, similar to how basic blocks are no operands of PHI nodes). At least the following tests failed, I didn't investigate. |
|
Thanks @aengelke for sharing the issue. I had a very quick look at the first test failure you were getting. It is triggering the assertion in If you are still actively working on the patch, I would be happy to spend some time to look into the crashes and locate the exact issues. |
|
Looks like an O2 problem, at O0, it fails as expected due to case_values() returning nullptr. I now spent ~1h looking at the code and still have no idea on how to fix this. SandboxIR seems to have its own ConstantInt class and I imagine that I would need to mirror the changes somehow in SandboxIR. In any case, I'm reasonably confident that the iterator is not broken by the change.
I'd appreciate that. I put up #166842, feel free to edit. |
5 participants
These components are still experimental, have not recieved commit traffic beyond basic maintenance since May, and it was decided that the people at Google working on the project should focus on other priorities. Given the lack of investment, delete the code for now. If someone wants to revive this, they can revert this patch. For now, delete it so that upstream does not have to bear the maintenance cost.