[SandboxIR] Refactor sandboxir::PHINode::removeIncomingValueIf() to delegate the removal directly to the underlying llvm::PHINode::removeIncomingValueIf

[Enna1]

No description provided.

[nikic]

I think this would actually interfere with #171963, because it uses a removeIncomingValueIf() and removeIncomingValue() use different removal strategies, so I think having a single PHIRemoveIncoming wouldn't be able to cover both?

[Enna1]

I think this would actually interfere with #171963, because it uses a removeIncomingValueIf() and removeIncomingValue() use different removal strategies, so I think having a single PHIRemoveIncoming wouldn't be able to cover both?

The intention behind this patch was that we don't need having KeepIncomingOrder flag for removeIncomingValue() in #171963 with this patch.

But think about this a bit, this is not enough. as you pointed out, with #171963, removeIncomingValueIf() and removeIncomingValue() will use different removal strategies, so we cannot cover them with a single PHIRemoveIncoming.

If we don't want the KeepIncomingOrder flag for PHINode::removeIncomingValue(), two version PHIRemoveIncoming are needed, we can move the KeepIncomingOrder flag into PHIRemoveIncoming...

What do you think?

[nikic]

If you keep the current removeIncomingValueIf() implementation, wouldn't you only need one implementation because it uses removeIncomingValue() internally? As it already does the walk backwards, it should be compatible with the swapping approach, right?

[Enna1]

If you keep the current removeIncomingValueIf() implementation, wouldn't you only need one implementation because it uses removeIncomingValue() internally? As it already does the walk backwards, it should be compatible with the swapping approach, right?

llvm-project/llvm/unittests/SandboxIR/SandboxIRTest.cpp

Lines 5954 to 5968 in 5891921

	// Check replaceIncomingValueIf
	EXPECT_EQ(PHI->getNumIncomingValues(), 5u);
	auto *RemainBB0 = PHI->getIncomingBlock(0);
	auto *RemoveBB0 = PHI->getIncomingBlock(1);
	auto *RemainBB1 = PHI->getIncomingBlock(2);
	auto *RemoveBB1 = PHI->getIncomingBlock(3);
	auto *RemainBB2 = PHI->getIncomingBlock(4);
	PHI->removeIncomingValueIf([&](unsigned Idx) {
	return PHI->getIncomingBlock(Idx) == RemoveBB0 ||
	PHI->getIncomingBlock(Idx) == RemoveBB1;
	});
	EXPECT_EQ(PHI->getNumIncomingValues(), 3u);
	EXPECT_EQ(PHI->getIncomingBlock(0), RemainBB0);
	EXPECT_EQ(PHI->getIncomingBlock(1), RemainBB1);
	EXPECT_EQ(PHI->getIncomingBlock(2), RemainBB2);

This test expects after removal the order of incoming values remained is preserved.

Current removeIncomingValueIf() implementation call removeIncomingValue() , assertion will fail if removeIncomingValue() switch to the swapping approach.

[nikic]

Wouldn't it be fine to change the test expectation? For SandboxIR, the main thing we care about is that the change can be reliably undone.