Page MenuHomePhabricator

[DAG] Fix PR45049: LegalizeTypes crash
ClosedPublic

Authored by jroelofs on Mar 28 2020, 12:04 PM.

Details

Summary

Sometimes LegalizeTypes knows about common subexpressions before SelectionDAG
does, leading to accidental SDValue removal before its reference count was
truly zero.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=45049

Diff Detail

Event Timeline

jroelofs created this revision.Mar 28 2020, 12:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 28 2020, 12:04 PM
bjope added a subscriber: bjope.Mar 28 2020, 2:04 PM

FYI: Looks like this solves https://bugs.llvm.org/show_bug.cgi?id=44546 as well.

tra resigned from this revision.Mar 30 2020, 9:41 AM

Looks OK to me, but I'm not that familiar with the legalizer.

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
191

Nit: for (auto const &I: ReplacedValues)

llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
204–205

Do we need an assert at the beginning of the loop that Old != New? Otherwise we may be removing a valid map entry.

jroelofs marked 2 inline comments as done.Mar 30 2020, 12:11 PM
jroelofs added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.h
204–205

Good idea. I believe that would only happen for an RAUW(N, N), which the DAGUpdateListener call sites guarantee won't happen.

jroelofs updated this revision to Diff 253660.Mar 30 2020, 12:11 PM
bjope added inline comments.Apr 4 2020, 7:47 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
191

Don't you need to wrap the whole for loop inside #ifndef NDEBUG. Otherwise I would be a def without any uses in a no-assert build.

Looking at the for loop above there is no such protection for UI (but maybe that is because UI is incremented in the loop).

I wouldn't mind if you added #ifndef NDEBUG around both loops, as they only make sense when the asserts are active.

jroelofs updated this revision to Diff 255047.Apr 4 2020, 9:01 AM

Fix NDEBUG build.

jroelofs marked 2 inline comments as done.Apr 4 2020, 9:01 AM
jroelofs added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp
191

Good catch!

bjope accepted this revision.Apr 4 2020, 9:51 AM

LGTM!

This revision is now accepted and ready to land.Apr 4 2020, 9:51 AM
jroelofs updated this revision to Diff 255522.Apr 6 2020, 3:48 PM
jroelofs edited the summary of this revision. (Show Details)

Reverted this in 3ce77142a6452d76d6f97c9a6c2da193e78841ba because one of the new assertions failed in the expensive-checks bots: http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-debian/builds/4971/steps/test-check-all/logs/FAIL%3A%20LLVM%3A%3A2008-12-02-IllegalResultType.ll

This patch removes that check, which apparently wasn't correct.

jroelofs reopened this revision.Apr 6 2020, 4:28 PM
This revision is now accepted and ready to land.Apr 6 2020, 4:28 PM

@bjope are you happy with me re-landing this with that new assert dropped? Looks like that case is properly covered earlier in PerformExpensiveChecks() anyway.

bjope added a comment.Apr 10 2020, 5:47 PM

@bjope are you happy with me re-landing this with that new assert dropped? Looks like that case is properly covered earlier in PerformExpensiveChecks() anyway.

Yes, that is fine with me!