[SandboxIR] Implement missing PHINode functions #101734
Conversation
replaceIncomingBlockWith and removeIncomingValueIf are both straightforward and done. I'll defer copyIncomingBlocks until a couple of other changes that also handle blocks go in.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
please prefix the commit title with |
llvm/lib/SandboxIR/SandboxIR.cpp
Outdated
| setIncomingBlock(Op, New); | ||
| } | ||
| void PHINode::removeIncomingValueIf(function_ref< bool(unsigned)> Predicate, | ||
| bool DeletePHIIfEmpty) { |
There was a problem hiding this comment.
Good catch, should be passed through to the llvmir function.
There was a problem hiding this comment.
Blah. It turns out that deleting empty PHINodes is unimplemented in the tracker right now, and needs cleanup in several places. I'll delete this use for now and fix it in a variety of places in a later follow up.
There was a problem hiding this comment.
I think we also need a test in TrackerTest.cpp (in PHINodeSetters test) to make sure that replaceIncomingBlockWith() and removeIncomingValueIf() can be reverted properly. They currently call functions that are being tracked, so tracking should work fine, but this may change, and we don't want tracking to fail silently.
Especially improve removal test.
| auto *NewBB = BB2; | ||
| EXPECT_NE(NewBB, OrigBB); | ||
| PHI->replaceIncomingBlockWith(OrigBB, NewBB); | ||
| EXPECT_EQ(PHI->getIncomingBlock(0), NewBB); |
There was a problem hiding this comment.
Let's also check that no other incoming block has changed.
| return PHI->getIncomingBlock(Idx) == NewBB; | ||
| }); | ||
| EXPECT_EQ(PHI->getNumIncomingValues(), 1u); | ||
| EXPECT_EQ(PHI->getIncomingBlock(0), BB3); | ||
| // Check create(). |
There was a problem hiding this comment.
I think we should exercise removeIncomingValueIf() a bit more. In the existing check we are removing the first two values if I am not mistaken: (NewBB, NewBB, BB3) -> (BB3).
Maybe we should try removing the last incoming value, resulting in an empty PHI, and perhaps a different pattern with gaps, like (A, B, C, D) -> (B, D).
| }); | ||
| EXPECT_EQ(PHI->getNumIncomingValues(), 1u); | ||
| Ctx.revert(); | ||
| EXPECT_EQ(PHI->getNumIncomingValues(), 2u); |
There was a problem hiding this comment.
It's probably best to check the actual incoming values here too, not just there are 2 of them
Update test for much more complicated case.
replaceIncomingBlockWith and removeIncomingValueIf are both straightforward and done.
I'll defer copyIncomingBlocks until a couple of other changes that also handle blocks go in.