This is an archive of the discontinued LLVM Phabricator instance.

[GVN] Invalidate MDA when deduplicating phi nodes
ClosedPublic

Authored by nikic on Aug 25 2023, 7:54 AM.

Details

Summary

Duplicate phi nodes were being directly removed, without invalidating MDA. This could result in a new phi node being allocated at the same address, incorrectly reusing a cache entry.

Fix this by optionally allowing EliminateDuplicatePHINodes() to collect phi nodes to remove into a vector, which allows GVN to handle removal itself.

Fixes https://github.com/llvm/llvm-project/issues/64598.

Diff Detail

Event Timeline

nikic created this revision.Aug 25 2023, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 7:54 AM
nikic requested review of this revision.Aug 25 2023, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 7:54 AM
nikic added inline comments.Aug 25 2023, 7:56 AM
llvm/test/Transforms/GVN/pr64598.ll
3

I got this test case by llvm-reducing between "good opt" and "bad opt", so I'm not sure how meaningful it is. I haven't pre-committed this test because address reuse might be flaky.

I would have preferred to instead expose the issue by adding an AssertingVH in MemDepAnalysis. Unfortunately, the problematic value is part of using ValueIsLoadPair = PointerIntPair<const Value *, 1, bool> and there is no easy way to use AssertingVH in there (even if switching to std::pair, given how it is used).

Carrot added inline comments.Aug 25 2023, 8:32 AM
llvm/include/llvm/Transforms/Utils/Local.h
169

Why not use SmallPtrSet?

nikic added inline comments.Aug 25 2023, 8:37 AM
llvm/include/llvm/Transforms/Utils/Local.h
169

I was trying to make sure the order is deterministic -- maybe it doesn't matter though?

Carrot added inline comments.Aug 25 2023, 9:33 AM
llvm/include/llvm/Transforms/Utils/Local.h
169

Each PHINode address is unique, and it is only used to check if a PHI instruction is contained in the set in function EliminateDuplicatePHINodes, and later delete all instructions in the set. So I think the order doesn't matter.

nikic updated this revision to Diff 553883.Aug 28 2023, 3:19 AM

Use SmallPtrSet instead of SmallSetVector.

fhahn added inline comments.Aug 28 2023, 4:39 AM
llvm/lib/Transforms/Utils/Local.cpp
1290

It might simplify the code in the static functions if ToRemove would be required for the static functions and have a loop to remove them in EliminateDuplicatePHINodes if the pointer is null?

llvm/test/Transforms/GVN/pr64598.ll
3

I sounds like the test as is to be the best option we have for now unfortunately

nikic updated this revision to Diff 553917.Aug 28 2023, 6:55 AM

Always use the set in the implementation, implement set-less overload on top of it.

Carrot accepted this revision.Sep 14 2023, 7:40 PM

Looks good to me.

This revision is now accepted and ready to land.Sep 14 2023, 7:40 PM
This revision was automatically updated to reflect the committed changes.