[PATCH] D157267: [NewGVN] Don't always update use counts for SSA copies
Vladimir Radosavljevic via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Aug 24 03:46:42 PDT 2023
vladimirradosavljevic added inline comments.
================
Comment at: llvm/lib/Transforms/Scalar/NewGVN.cpp:4119
if (isSSACopy) {
unsigned &IIUseCount = UseCounts[II];
if (--IIUseCount == 0)
----------------
kmitropoulou wrote:
> kmitropoulou wrote:
> > vladimirradosavljevic wrote:
> > > kmitropoulou wrote:
> > > > The test case does not crash in my workspace. Anyway, I think the problem is here. If II is not in the UseCounts map, then it just adds it and it returns zero. Will the following change solve the problem?
> > > >
> > > > if (isSSACopy) {
> > > > auto It = UseCounts.find(II);
> > > > if (It != UseCounts.end()) {
> > > > unsigned &IIUseCount = It->second;
> > > > if (--IIUseCount == 0)
> > > > ProbablyDead.insert(II);
> > > > }
> > > > }
> > > >
> > > Could you please try to run it with valgrind/sanitizers, because it happens occasionally and this is why it is such a dangerous bug.
> > > Yes, proposed solution fixes the problem. Please note that I'm not familiar with the NewGVN pass, but if II is not in UseCounts map, should we also insert it into ProbablyDead?
> > I am not expert on this part of NewGVN, but I think that if II is in UseCounts map, then II should be already in ProbablyDead. This logic is towards the bottom of convertClassToDFSOrdered() .
> >
> > Regarding the test, if it crashes with the sanitizer, then it is OK. But, the test is way too big. Please make it smaller.
> >
> > Since NewGVN is not enabled by default, we should run the tests from test-suite with NewGVN enabled. Please be aware that there are some tests that are failing due to NewGVN.
> I am sorry I meant:
>
> I think that if II is not in UseCounts map, then II should be already in ProbablyDead.
Thanks for the clarification! I submitted the new patch with your suggestion. Regarding the test case, it is the smallest reproducer I managed to create, and I used llvm-reduce test to do so.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157267/new/
https://reviews.llvm.org/D157267
More information about the llvm-commits
mailing list