[SandboxIR] Add more Instruction member functions #98588
Conversation
llvm/lib/SandboxIR/SandboxIR.cpp
Outdated
| return; | ||
| auto *LLVMBB = cast<llvm::BasicBlock>(BB.Val); | ||
| llvm::BasicBlock::iterator It; | ||
| if (WhereIt == BB.end()) |
llvm/lib/SandboxIR/SandboxIR.cpp
Outdated
| } | ||
|
|
||
| std::unique_ptr<Value> Context::detach(Value *V) { | ||
| #ifndef NDEBUG |
There was a problem hiding this comment.
assert(V->getSubclassID() != Constant && "");
assert(V->getSubclassID() != User && "");
is much shorter
| return BasicBlock::iterator(I->getParent(), I->getIterator(), &Ctx); | ||
| } | ||
|
|
||
| Instruction *Instruction::getNextNode() const { |
There was a problem hiding this comment.
is this necessary? this is part of ilist_node_with_parent, so the name doesn't really make sense here. if we do want to support this operation, I'd at least rename it to something like getNextInstruction()
There was a problem hiding this comment.
Well getNextNode() is part of the Instruction API, so the user expects it to be there. So I am not sure we should remove it, or use a different name for a function that does the same thing. Perhaps we could have both getNextInstruction() and getNextNode()? Wdyt?
There was a problem hiding this comment.
I still think that Node is an implementation detail and we can just have getNextInstruction(), but don't feel that strongly. This is fine for now.
| assert(getIterator() != getParent()->end() && "Already at end!"); | ||
| auto *LLVMI = cast<llvm::Instruction>(Val); | ||
| assert(LLVMI->getParent() != nullptr && "LLVM IR instr is detached!"); | ||
| auto *NextLLVMI = LLVMI->getNextNode(); |
There was a problem hiding this comment.
what about multi-instruction instructions?
There was a problem hiding this comment.
So here is how this works: NextLLVMI is an LLVM instruction that belongs to the next SandboxIR instruction, whether it's a single-Instruction or multi-Instruction. If it's a multi-Instruction, then NextLLVMI will be the topmost LLVM instruction. In either case Ctxt.getValue(NextLLVMI) returns the correct SandboxIR Instruction that maps to NextLLVMI. Does this make sense?
There was a problem hiding this comment.
I meant if this is a multi-instruction, then NextLLVMI will point to the second LLVM instruction in this. Or are you going to override getNextNode() in the multi-instruction subclass?
There was a problem hiding this comment.
Ah the confusing part is that Val always points to the bottom LLVM Instruction of a sandboxir::Instruction, not the topmost one.
So if this is a multi-instruction, LLVMI is the bottom-most LLVM Instruction in this, so LLVMI->getNextNode() points to an LLVM instruction that does not belong to this, but instead to the next sandboxir::Instruction.
There was a problem hiding this comment.
oh I see. can you update the comments around Val (separate PR) to say that?
|
README.md would be great, but could you put a minimal SandboxIR.rst file in https://github.com/llvm/llvm-project/tree/main/llvm/docs |
I agree, there is no information about the design at this point. Perhaps an |
|
in |
|
rst is the default. There is no markdown. |
there are multiple |
|
The README.md states |
llvm/lib/SandboxIR/SandboxIR.cpp
Outdated
| It = WhereI->getTopmostLLVMInstruction()->getIterator(); | ||
| } | ||
| auto IRInstrsInProgramOrder(getLLVMInstrs()); | ||
| sort(IRInstrsInProgramOrder, |
There was a problem hiding this comment.
should we enforce that getLLVMInstrs() returns the instructions in program order so we don't need to sort here?
There was a problem hiding this comment.
Yes, I think it makes sense, but what is a good way of enforcing it? I guess we can always add an assert(is_sorted()) in moveBefore().
There was a problem hiding this comment.
maybe in some verify method?
There was a problem hiding this comment.
Yeah, that should work.
There was a problem hiding this comment.
Supporting this in the verifier is not trivial, so I will keep an assertion in moveBefore() for now.
llvm/lib/SandboxIR/SandboxIR.cpp
Outdated
| Instruction *BeforeI; | ||
| llvm::BasicBlock::iterator LLVMBeforeIt; | ||
| if (WhereIt != BB->end()) { | ||
| BeforeI = &*WhereIt; |
| assert(getIterator() != getParent()->end() && "Already at end!"); | ||
| auto *LLVMI = cast<llvm::Instruction>(Val); | ||
| assert(LLVMI->getParent() != nullptr && "LLVM IR instr is detached!"); | ||
| auto *NextLLVMI = LLVMI->getNextNode(); |
There was a problem hiding this comment.
I meant if this is a multi-instruction, then NextLLVMI will point to the second LLVM instruction in this. Or are you going to override getNextNode() in the multi-instruction subclass?
| assert(getIterator() != getParent()->end() && "Already at end!"); | ||
| auto *LLVMI = cast<llvm::Instruction>(Val); | ||
| assert(LLVMI->getParent() != nullptr && "LLVM IR instr is detached!"); | ||
| auto *NextLLVMI = LLVMI->getNextNode(); |
There was a problem hiding this comment.
oh I see. can you update the comments around Val (separate PR) to say that?
| return BasicBlock::iterator(I->getParent(), I->getIterator(), &Ctx); | ||
| } | ||
|
|
||
| Instruction *Instruction::getNextNode() const { |
There was a problem hiding this comment.
I still think that Node is an implementation detail and we can just have getNextInstruction(), but don't feel that strongly. This is fine for now.
| @@ -623,6 +670,12 @@ class Context { | |||
| DenseMap<llvm::Value *, std::unique_ptr<sandboxir::Value>> | |||
| LLVMValueToValueMap; | |||
|
|
|||
| /// Remove \p V from the maps and returns the unique_ptr. | |||
| std::unique_ptr<Value> detachLLVMValue(llvm::Value *V); | |||
There was a problem hiding this comment.
I'm debating whether or not this should have [[nodiscard]]. Unsure how pervasive you'd want [[nodiscard]] to be throughout these APIs
There was a problem hiding this comment.
I don't feel strongly either way, but these detach() functions are meant to be used either for detaching or for just deleting the value. I think it's currently only used for deleting them, so the value is discarded. There is no real harm in discarding the returned value as far as I can tell.
There was a problem hiding this comment.
I thought we wanted to keep around Values in case we rollback
There was a problem hiding this comment.
These APIs are for internal use only. The user won't be able to detach values. The only way a user can delete a value is through the eraseFromParent() API, and this will be tracked and can be rolled back.
This patch adds new Instruction member functions, including: - getNextNode() - getPrevNode() - getOpcode() - removeFromParent() - eraseFromParent() - getParent() - insertBefore() - insertAfter() - insertInto() - moveBefore() - moveAfter()
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/1767 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/2424 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/2238 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/1768 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/1817 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/1839 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/794 Here is the relevant piece of the build log for the reference: |
This patch adds new Instruction member functions, including: - getNextNode() - getPrevNode() - getOpcode() - removeFromParent() - eraseFromParent() - getParent() - insertBefore() - insertAfter() - insertInto() - moveBefore() - moveAfter()
Summary: This reverts commit 618b0b7. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251723
Summary: This reverts commit 1ca07ce. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251476
This patch adds new Instruction member functions, including: