[SandboxIR][Tracker] Track eraseFromParent() #99431
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| SmallVector<InstrData> InstrData; | ||
| /// This is either the next Instruction in the stream, or the parent | ||
| /// BasicBlock if at the end of the BB. | ||
| llvm::Value *NextLLVMIOrBB; |
There was a problem hiding this comment.
I would have used a std::variant to make it absolutely clear that it is either or and the compiler verifies that invariant.
There was a problem hiding this comment.
Hmm isn't there an ADT variant for pointers that has a smaller footprint than std::variant ?
There was a problem hiding this comment.
I think it's the PointerUnion, let me try to use that.
vporpo
force-pushed
the
SBVec
branch
2 times, most recently
from
387c241 to
a3606d8
Compare
llvm/lib/SandboxIR/Tracker.cpp
Outdated
| OpDataVec.push_back({Use.get(), static_cast<unsigned>(OpNum)}); | ||
| InstrData.push_back({OpDataVec, LLVMI}); | ||
| } | ||
| #ifndef NDEBUG |
There was a problem hiding this comment.
no need for ifdef NDEBUG, the code should optimize away in a non-asserts build. unsure if need to [[maybe_unused]] Idx
There was a problem hiding this comment.
I replaced it with assert(is_sorted())
| for (llvm::Instruction *I : LLVMInstrs) | ||
| I->dropAllReferences(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
imo putting an else here rather than the return is a bit clearer
| /// The operand and the corresponding operand number. | ||
| struct OpData { | ||
| llvm::Value *Op; | ||
| unsigned OpNum; |
There was a problem hiding this comment.
do we need this? can we just use the index from enumerate(OpDataVec)?
There was a problem hiding this comment.
You are right, we are saving/restoring all of them so no need for this.
| }; | ||
| /// The instruction data is in reverse program order, which helps create the | ||
| /// original program order during revert(). | ||
| SmallVector<InstrData> InstrData; |
There was a problem hiding this comment.
we can't test the multi-instruction code because we don't have an multi-instructions right now right? we should figure out a way to test that. maybe a test extension with a multi-instruction?
There was a problem hiding this comment.
Yeah, multi-instructions are in the vectorizer-specific extension of SandboxIR. The easiest way would be to just re-test the tracker with multi-instructions once they get introduced. This is what I've done in the prototype.
Not sure how we would exercise the code without introducing some kind of dummy multi-instruction just for testing. But that would have to get registered properly in the def file, and we would need to enforce that it's not created by the user.
Any thoughts?
There was a problem hiding this comment.
if we add a test extension it should be pretty clear that it's not for users. that would also be a nice way to show how to write an extension that doesn't contain all the vectorization complexity.
but I'm ok with adding a TODO to test multi-instructions and filling that in in a separate PR that adds a test extension
There was a problem hiding this comment.
OK we can a new context class, like class TestContext : public Context that can match a multi-instruction pattern like for example a 2-instruction sequence of a sub followed by an add. Then in the tests we need to test multi-instructions we can use that TestContext instead of the standard Context.
Yeah that would have to be a separate PR. I am pretty sure there is a lot of missing functionality in the SandboxIR API that would need fixing and more testing too once we introduce multi-instructions.
This patch adds tracking support for Instruction::eraseFromParent(). The Instruction is not actually being erased, but instead it is detached from the instruction list and drops its Use edges. The original instruction position and Use edges are saved in the `EraseFromParent` change object, and are being used during `revert()` to restore the original state.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/2097 Here is the relevant piece of the build log for the reference: |
Summary: This patch adds tracking support for Instruction::eraseFromParent(). The Instruction is not actually being erased, but instead it is detached from the instruction list and drops its Use edges. The original instruction position and Use edges are saved in the `EraseFromParent` change object, and are being used during `revert()` to restore the original state. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250958
This patch adds tracking support for Instruction::eraseFromParent(). The Instruction is not actually being erased, but instead it is detached from the instruction list and drops its Use edges. The original instruction position and Use edges are saved in the
EraseFromParentchange object, and are being used duringrevert()to restore the original state.