[PATCH] D157267: [NewGVN] Don't always update use counts for SSA copies

[Vladimir Radosavljevic via Phabricator via llvm-commits]

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