Page MenuHomePhabricator

PHINode: introduce setIncomingValueForBlock() function, and use it.
ClosedPublic

Authored by Whitney on Jun 14 2019, 9:00 AM.

Details

Summary

There is PHINode::getBasicBlockIndex() and PHINode::setIncomingValue() but no function to replace incoming value for a specified BasicBlock* predecessor.
Clearly, there are a lot of places that could use that functionality.

Diff Detail

Repository
rL LLVM

Event Timeline

Whitney created this revision.Jun 14 2019, 9:00 AM
fhahn added a subscriber: fhahn.Jun 14 2019, 9:15 AM
fhahn added inline comments.
llvm/include/llvm/IR/Instructions.h
2729 ↗(On Diff #204779)

can this just be

int Idx = getBasicBlockIndex(BB);
assert(Idx >= 0 && "Invalid basic block argument!");
return setIncomingValue(Idx, V);

similar to getIncomingValueForBlock above?

Whitney marked an inline comment as done.Jun 14 2019, 9:28 AM
Whitney added inline comments.
llvm/include/llvm/IR/Instructions.h
2729 ↗(On Diff #204779)

I thought of doing that, but sometimes a phi node may have multiple operands of the same incoming block (not sure why). I am using a similar code pattern as in replaceIncomingBlockWith().

fhahn added inline comments.Jun 14 2019, 9:44 AM
llvm/include/llvm/IR/Instructions.h
2729 ↗(On Diff #204779)

Ah right. I think it is worth mentioning in the doc comment that it will replace all values coming from BB?

For the places that use getbasicBlockIndex, this is not a NFC I think, as they would only replace the incoming value of the first occurrence.

Whitney updated this revision to Diff 204801.Jun 14 2019, 9:55 AM
Whitney retitled this revision from [NFC] PHINode: introduce setIncomingValueForBlock() function, and use it. to PHINode: introduce setIncomingValueForBlock() function, and use it..
Whitney marked an inline comment as done.
Whitney added inline comments.
llvm/include/llvm/IR/Instructions.h
2729 ↗(On Diff #204779)

Removed [NFC] from the title. And updated the comment.

Meinersbur accepted this revision.Jun 14 2019, 1:31 PM

LGTM, please wait for @fhahn's ok as well.

llvm/include/llvm/IR/Instructions.h
2729 ↗(On Diff #204779)

It's not forbidden and sometimes cannot even be simplified by fusing edges, e.g. a switch with multiple values branching to the same block.

Unfortunately for the PHI it is indistinguishable which of the switch cases was taken. That means, all of the incoming values from the same block should be the same. This patch may actually fix some bugs where only one of the incoming values was changed.

2677 ↗(On Diff #204801)

[nit] unrelated

2733 ↗(On Diff #204801)

[suggestion] assert when no matching block was found?

This revision is now accepted and ready to land.Jun 14 2019, 1:31 PM
fhahn accepted this revision.Jun 17 2019, 4:08 AM

LGTM, thanks for updating. I think it would be good to add an assertion as Micheal suggested, as we have similar ones in related functions.

Whitney updated this revision to Diff 205065.Jun 17 2019, 7:15 AM

Added the assert. Thanks for the reviews.

This revision was automatically updated to reflect the committed changes.