This is an archive of the discontinued LLVM Phabricator instance.

[SROA] Handle PHI with multiple duplicate predecessors
ClosedPublic

Authored by bjope on May 4 2018, 5:28 AM.

Details

Summary

The verifier accepts PHI nodes with multiple entries for the
same basic block, as long as the value is the same.

As seen in PR37203, SROA did not handle such PHI nodes properly
when speculating loads over the PHI, since it inserted multiple
loads in the predecessor block and changed the PHI into having
multiple entries for the same basic block, but with different
values.

This patch teaches SROA to reuse the same speculated load for
each PHI duplicate entry in such situations.

Resolves: https://bugs.llvm.org/show_bug.cgi?id=37203

Diff Detail

Event Timeline

bjope created this revision.May 4 2018, 5:28 AM

At least the fix solves the problem I saw but I don't know SROA enough to say if the fix is good or not.

So the patch has been confirmed to solve the PR (thanks @uabelho).
But still no ready-to-land. Well I think that I'll land this anyway tomorrow, to get rid of the bug.

But still no ready-to-land. Well I think that I'll land this anyway tomorrow, to get rid of the bug.

This is not how the review process works; we expect pre-commit review for non-trivial patches.

lib/Transforms/Scalar/SROA.cpp
1264

We generally use DenseMap in LLVM code, not unordered_map (see http://llvm.org/docs/ProgrammersManual.html#map-like-containers-std-map-densemap-etc ).

I don't think it's a good idea to make the rest of the code more complicated for the sake of the "PHI node has multiple entries" assertion.

1274

DenseMap has a method "lookup" you can use to avoid the iterator.

  1. If this is common enough, should we just have a unique_predecessors iterator?

(unfortunately, std::unique won't work here because preds are not sorted)

  1. I don't think it's worth issuing an assert here if the verifier already verifies it.

If we don't add a unique_processor, can you just use SmallPtrSet<BasicBlock *> HandledPreds, and insert/check whether the pred has been handled?

Oh looks like Eli beat me by a few minutes :)

Not sure unique_predecessors would help here; we have to call PHINode::addIncoming once for each predecessor, whether or not it's unique.

Ugh, yes, i was focused on the error issuing part and not the hey you have to create real phi nodes part.

bjope added inline comments.May 16 2018, 1:18 PM
lib/Transforms/Scalar/SROA.cpp
1274

So if I have a

DenseMap<BasicBlock*, Value*> Map;

can I do

Value* V = Map.lookup(Pred);

and expect to get a null pointer in case Pred isn't found?

bjope updated this revision to Diff 147173.May 16 2018, 1:55 PM

Replaced the unordered_map by a DenseMap.

bjope marked 2 inline comments as done.May 16 2018, 1:57 PM

Thanks Eli. Definitely a cleaner solution when using a DenseMap.

efriedma accepted this revision.May 16 2018, 2:13 PM

LGTM

lib/Transforms/Scalar/SROA.cpp
1274

Yes, lookup() returns null if the key isn't in the map.

This revision is now accepted and ready to land.May 16 2018, 2:13 PM
This revision was automatically updated to reflect the committed changes.