This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Avoid modifying AdditionalUsers while iterating over it
ClosedPublic

Authored by dim on Mar 14 2021, 9:46 AM.

Details

Summary

When run under valgrind, or with a malloc that poisons freed memory,
this can lead to segfaults or other problems.

To avoid modifying the AdditionalUsers DenseMap while still iterating,
save the instructions to be notified in a separate SmallPtrSet, and use
this to later call OperandChangedState on each instruction.

Fixes PR49582.

Diff Detail

Event Timeline

dim created this revision.Mar 14 2021, 9:46 AM
dim requested review of this revision.Mar 14 2021, 9:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2021, 9:46 AM

Test missing.

Am i understanding correctly that the problem is *only* that the inner SmallPtrSet may get new entries added to it?
I think the fix depends on the expected behavior here, in particular, two things stand out:

  • when looping over that set there, if a new unique entry is added in process, should it be ignored, or handled?
  • what if said new entry isn't unique? should it be re-handled?

I'm not sure about the latter, but not handling the former seems like a bug.
I think we should instead change AdditionalUsers from DenseMap<Value *, SmallPtrSet<User *, 2>> to DenseMap<Value *, SetVector<User *, 2>>,
and change the loop to

// NOTE: new entries may be added to the vector in-process, invalidating iterators.
for(int I = 0; I != Iter->second.size(); ++I) {
  User *U = Iter->second[I];
  <...>
}

But it is also possible that i don't know what i'm talking about.

Does the crash trigger with ASan/MSan? Would it be possible to add a reduced test case from the bug report?

dim added a comment.Mar 14 2021, 11:02 AM

Test missing.

I haven't yet reduced the test case, will see to that. (One issue is that any failure would only show up on FreeBSD and OpenBSD...)

Am i understanding correctly that the problem is *only* that the inner SmallPtrSet may get new entries added to it?

I think so, the problem is that SCCPSolver::AdditionalUsers is a DenseMap which is iterated over:

auto Iter = AdditionalUsers.find(I);
if (Iter != AdditionalUsers.end()) {
  for (User *U : Iter->second)
    if (auto *UI = dyn_cast<Instruction>(U))
      OperandChangedState(UI);
}

meanwhile, OperandChangedState() can change the Instruction pointed to, via visit():

void OperandChangedState(Instruction *I) {
  if (BBExecutable.count(I->getParent()))   // Inst is executable?
    visit(*I);
}

Apparently, visit() can cause the SmallPtrSet to grow, which invalidates the U iterator, as it shows in valgrind output:

==1366860== Invalid read of size 8
==1366860==    at 0x18CD850: AdvanceIfNotValid (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:250)
==1366860==    by 0x18CD850: operator++ (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:301)
==1366860==    by 0x18CD850: (anonymous namespace)::SCCPSolver::markUsersAsChanged(llvm::Value*) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:536)
==1366860==    by 0x18C773A: (anonymous namespace)::SCCPSolver::Solve() (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1420)
==1366860==    by 0x18C4EE4: llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>, llvm::function_ref<llvm::AnalysisResultsForFn (llvm::Function&)>) (sr
c/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:0)
==1366860==    by 0x153E53E: llvm::IPSCCPPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/lib/Transforms/IPO/SCCP.cpp:24)
==1366860==    by 0x28BF9BC: llvm::detail::PassModel<llvm::Module, llvm::IPSCCPPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm
-project/llvm/include/llvm/IR/PassManagerInternal.h:85)
==1366860==    by 0x13E9D75: llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/include/llvm/IR/PassManager.h:517)
==1366860==    by 0x1C254A7: (anonymous namespace)::EmitAssemblyHelper::EmitAssemblyWithNewPassManager(clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (src/
llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1465)
==1366860==    by 0x1C1F9CF: clang::EmitBackendOutput(clang::DiagnosticsEngine&, clang::HeaderSearchOptions const&, clang::CodeGenOptions const&, clang::TargetOptions const&, clang::LangOptions const&, llvm::DataLayout
const&, llvm::Module*, clang::BackendAction, std::unique_ptr<llvm::raw_pwrite_stream, std::default_delete<llvm::raw_pwrite_stream> >) (src/llvm/llvm-project/clang/lib/CodeGen/BackendUtil.cpp:1634)
==1366860==    by 0x27FE6DF: clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) (src/llvm/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:334)
==1366860==    by 0x32CF2C2: clang::ParseAST(clang::Sema&, bool, bool) (src/llvm/llvm-project/clang/lib/Parse/ParseAST.cpp:171)
==1366860==    by 0x21D8AC3: clang::FrontendAction::Execute() (src/llvm/llvm-project/clang/lib/Frontend/FrontendAction.cpp:949)
==1366860==    by 0x2154162: clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) (src/llvm/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:955)
==1366860==  Address 0x7c9c678 is 8 bytes inside a block of size 1,024 free'd
==1366860==    at 0x697BA3F: free (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1366860==    by 0x197F06A: llvm::SmallPtrSetImplBase::Grow(unsigned int) (src/llvm/llvm-project/llvm/lib/Support/SmallPtrSet.cpp:116)
==1366860==    by 0x197EEF5: llvm::SmallPtrSetImplBase::insert_imp_big(void const*) (src/llvm/llvm-project/llvm/lib/Support/SmallPtrSet.cpp:0)
==1366860==    by 0xDA841E: insert_imp (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:154)
==1366860==    by 0xDA841E: llvm::SmallPtrSetImpl<llvm::User*>::insert(llvm::User*) (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:365)
==1366860==    by 0x18CF349: (anonymous namespace)::SCCPSolver::addAdditionalUser(llvm::Value*, llvm::User*) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:514)
==1366860==    by 0x18CE9BB: (anonymous namespace)::SCCPSolver::handleCallResult(llvm::CallBase&) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1332)
==1366860==    by 0x18D3F5B: (anonymous namespace)::SCCPSolver::visitCallBase(llvm::CallBase&) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1188)
==1366860==    by 0x18CD827: OperandChangedState (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:508)
==1366860==    by 0x18CD827: (anonymous namespace)::SCCPSolver::markUsersAsChanged(llvm::Value*) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:538)
==1366860==    by 0x18C773A: (anonymous namespace)::SCCPSolver::Solve() (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1420)
==1366860==    by 0x18C4EE4: llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>, llvm::function_ref<llvm::AnalysisResultsForFn (llvm::Function&)>) (sr
c/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:0)
==1366860==    by 0x153E53E: llvm::IPSCCPPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/lib/Transforms/IPO/SCCP.cpp:24)
==1366860==    by 0x28BF9BC: llvm::detail::PassModel<llvm::Module, llvm::IPSCCPPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm
-project/llvm/include/llvm/IR/PassManagerInternal.h:85)
==1366860==  Block was alloc'd at
==1366860==    at 0x697A7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1366860==    by 0x197EF77: safe_malloc (src/llvm/llvm-project/llvm/include/llvm/Support/MemAlloc.h:26)
==1366860==    by 0x197EF77: llvm::SmallPtrSetImplBase::Grow(unsigned int) (src/llvm/llvm-project/llvm/lib/Support/SmallPtrSet.cpp:100)
==1366860==    by 0x197EEF5: llvm::SmallPtrSetImplBase::insert_imp_big(void const*) (src/llvm/llvm-project/llvm/lib/Support/SmallPtrSet.cpp:0)
==1366860==    by 0xDA841E: insert_imp (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:154)
==1366860==    by 0xDA841E: llvm::SmallPtrSetImpl<llvm::User*>::insert(llvm::User*) (src/llvm/llvm-project/llvm/include/llvm/ADT/SmallPtrSet.h:365)
==1366860==    by 0x18CF349: (anonymous namespace)::SCCPSolver::addAdditionalUser(llvm::Value*, llvm::User*) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:514)
==1366860==    by 0x18CE9BB: (anonymous namespace)::SCCPSolver::handleCallResult(llvm::CallBase&) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1332)
==1366860==    by 0x18D3F5B: (anonymous namespace)::SCCPSolver::visitCallBase(llvm::CallBase&) (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1188)
==1366860==    by 0x18C7A6B: visit<llvm::ilist_iterator<llvm::ilist_detail::node_options<llvm::Instruction, true, false, void>, false, false> > (src/llvm/llvm-project/llvm/include/llvm/IR/InstVisitor.h:90)
==1366860==    by 0x18C7A6B: visit (src/llvm/llvm-project/llvm/include/llvm/IR/InstVisitor.h:105)
==1366860==    by 0x18C7A6B: visit (src/llvm/llvm-project/llvm/include/llvm/IR/InstVisitor.h:111)
==1366860==    by 0x18C7A6B: (anonymous namespace)::SCCPSolver::Solve() (src/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:1448)
==1366860==    by 0x18C4EE4: llvm::runIPSCCP(llvm::Module&, llvm::DataLayout const&, std::function<llvm::TargetLibraryInfo const& (llvm::Function&)>, llvm::function_ref<llvm::AnalysisResultsForFn (llvm::Function&)>) (sr
c/llvm/llvm-project/llvm/lib/Transforms/Scalar/SCCP.cpp:0)
==1366860==    by 0x153E53E: llvm::IPSCCPPass::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/lib/Transforms/IPO/SCCP.cpp:24)
==1366860==    by 0x28BF9BC: llvm::detail::PassModel<llvm::Module, llvm::IPSCCPPass, llvm::PreservedAnalyses, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm
-project/llvm/include/llvm/IR/PassManagerInternal.h:85)
==1366860==    by 0x13E9D75: llvm::PassManager<llvm::Module, llvm::AnalysisManager<llvm::Module>>::run(llvm::Module&, llvm::AnalysisManager<llvm::Module>&) (src/llvm/llvm-project/llvm/include/llvm/IR/PassManager.h:517)

I think the fix depends on the expected behavior here, in particular, two things stand out:

  • when looping over that set there, if a new unique entry is added in process, should it be ignored, or handled?
  • what if said new entry isn't unique? should it be re-handled?

I'm not sure about the latter, but not handling the former seems like a bug.
I think we should instead change AdditionalUsers from DenseMap<Value *, SmallPtrSet<User *, 2>> to DenseMap<Value *, SetVector<User *, 2>>,
and change the loop to

// NOTE: new entries may be added to the vector in-process, invalidating iterators.
for(int I = 0; I != Iter->second.size(); ++I) {
  User *U = Iter->second[I];
  <...>
}

But it is also possible that i don't know what i'm talking about.

I'm not familiar enough with the semantics of this code to have an informed opinion. :)

The solution proposed by @mortimer was just trying to work around the iterator invalidation, but preserves the original logic. In that respect, it is a minimalistic fix, but if there is a better solution, I'm all for it.

Does the crash trigger with ASan/MSan? Would it be possible to add a reduced test case from the bug report?

I've only run it under valgrind, but I'll attempt to reduce a test case from the bug report. On FreeBSD it gets a SIGBUS, and on OpenBSD apparently a SIGSEGV, but this type of memory error is typically non-portable. :)

dim added a comment.Mar 15 2021, 12:11 PM

Hm, I've reduced the test case to the following C++ source:

// clang -cc1 -triple amd64-- -S -O1 lastransform-min.cpp
int a(int, int);
int b(int, int, int);
int *c;
int d;
int h(int i) {
  int e = 1;
  for (; e < i;)
    if (c)
      if (c[0]) {
        if (e + 1 >= i)
          c[e + 1];
        if (e + 1 >= i)
          c[e + 1];
        if (e + 1 >= i)
          c[e + 1];
        if (e + 3 >= i)
          e += 3;
        if (e >= i)
          e++;
        if (e + 2 >= i)
          e += 2;
        if (e >= i)
          c[e];
        if (e >= i)
          c[e];
        if (e >= i)
          e++;
        if (e >= i)
          e++;
        if (c) {
          if (e >= i)
            return 0;
        } else if (d) {
          if (e >= i)
            e++;
          if (e + 1 >= i)
            e++;
        } else if (e + 2 >= i)
          c[e + 2];
      } else if (b(0, 0, 0)) {
        if (e + 3 >= i)
          c[e + 3];
        if (e + 3 >= i)
          c[e + 3];
        if (e + 3 >= i)
          e += 3;
        if (e >= i)
          e++;
        if (e + 1 >= i)
          e++;
        if (e >= i)
          c[e];
        if (e >= i)
          c[e];
        if (e >= i)
          c[0];
      } else if (b(0, 0, 0))
        if (b(0, 0, 0)) {
          if (e + 1 >= i)
            e++;
          if (e >= i)
            c[e];
          if (e >= i)
            c[e];
          if (e >= i)
            c[e];
          if (e >= i)
            c[e];
          else if (i)
            c[e];
          if (e >= i)
            return 0;
        } else if (c) {
          if (e + 1 >= i)
            c[e + 1];
          if (e + 1 >= i)
            c[e + 1];
          if (e + 1 >= i)
            c[e + 1];
          if (e + 1 >= i)
            c[e + 1];
        } else if (d) {
          if (e + 1 >= i)
            c[e + 1];
        } else if (a(0, 0)) {
          if (e + 1 >= i) {
            c[e + 1];
            return 0;
          }
          int f[e + 1];
        } else if (c) {
          if (e + 1 >= i)
            c[e + 1];
        } else if (b(0, 0, 0)) {
          if (e + 1 >= i)
            c[e + 1];
          if (e + 1 >= i)
            c[e + 1];
        } else if (c) {
          if (e + 1 >= i) {
            c[e + 1];
            return 0;
          }
          int g[e + 1];
        } else if (d) {
          if (e + 1 >= i)
            c[e + 1];
        } else if (a(0, 0)) {
          if (e + 1 >= i)
            c[e + 1];
        } else if (c) {
          if (e + 2 >= i)
            e += 2;
        } else if (b(0, 0, 0))
          if (c) {
            if (e + 3 >= i)
              e += 3;
          } else {
            if (e + 4 >= i)
              c[e + 4];
          }
        else if (c) {
          if (e + 1 >= i)
            c[e + 1];
          else if (i)
            c[e + 1];
        } else if (b(0, 0, 0)) {
          if (e + 2 >= i)
            e += 2;
          if (e >= i)
            e++;
          if (e >= i)
            e++;
          if (e >= i)
            e++;
          if (b(0, 0, 0)) {
            if (e >= i)
              c[e];
            if (e >= i)
              c[e];
            else
              c[e + 0];
            if (e >= i)
              c[e];
            if (e >= i)
              c[e];
            if (e >= i)
              c[e];
            if (e >= i)
              c[e];
            if (e >= i)
              c[e];
            else
              c[e + 0];
            if (e + 1 >= i)
              c[e + 1];
          } else if (a(0, 0)) {
            if (e + 1 >= i)
              c[e + 1];
          } else if (a(0, 0)) {
            if (e + 1 >= i)
              c[e + 1];
          } else if (a(0, 0)) {
            if (e + 3 >= i)
              c[e + 3];
          } else if (a(0, 0)) {
            if (e + 1 >= i)
              e++;
          } else if (a(0, 0)) {
            if (e)
              if (e >= i)
                c[0];
          } else if (a(0, 0)) {
            if (e + 1 >= i)
              e++;
          } else if (e + 2 >= i)
            c[e + 2];
        } else if (b(0, 0, 0))
          if (a(0, 0)) {
            if (e + 1 >= i)
              e++;
          } else if (a(0, 0)) {
            if (e + 1 >= i)
              e++;
          } else if (e + 1 >= i)
            e++;
          else {
            if (i)
              e++;
          }
        else if (b(0, 0, 0)) {
          if (e + 1 >= i)
            c[e + 1];
          if (e + 1 >= i)
            c[e + 1];
          if (e + 1 >= i)
            c[e + 1];
          if (e + 2 >= i)
            c[e + 2];
          if (e + 2 >= i)
            c[e + 2];
          if (e + 1 >= i)
            c[e + 1];
          else {
            0 >= i;
            c[e + 1];
          }
          if (e + 1 >= i)
            e++;
        } else if (b(0, 0, 0)) {
          if (e + 2 >= i)
            c[e + 2];
          else if (i)
            c[e + 2];
          if (e + 1 >= i)
            c[e + 1];
        } else if (b(0, 0, 0))
          if (e + 1 >= i)
            c[e + 1];
          else if (i)
            c[e + 1];
}

but I have not been able to change this into a .ll file that works in the context of llvm/test/Transforms/SCCP, where it's mostly .ll run through opt -sccp or opt -ipsccp.

I can't get opt to show invalid reads in valgrind though, these only show up when compiling the .cpp file with clang, as shown above in the comment.

Can anybody give me a clue on how to run opt so it performs the same optimization(s) as clang does?

dim added a comment.Mar 20 2021, 12:42 PM

Ok, I have not been able to make either llc or opt show any addressanitizer errors with the .ll output from the .cpp test case. Is it OK to check in a .cpp test case instead? That would require clang at regression test time though, and I'm unsure if that is available?

dim updated this revision to Diff 332128.Mar 20 2021, 2:09 PM

Added a bugpoint-reduced test case. (I could only get clang and opt to
trigger errors on a Linux machine by building with
LLVM_USE_SANITIZER=Address.)

dim updated this revision to Diff 333401.Mar 25 2021, 12:56 PM

My previous update accidentally dropped the changes to SCCP.cpp.
Squashed both local revisions to get both the functionality change and
the test case.

fhahn added a comment.Apr 1 2021, 7:39 AM
In D98602#2651521, @dim wrote:

My previous update accidentally dropped the changes to SCCP.cpp.
Squashed both local revisions to get both the functionality change and
the test case.

Thanks for adding the test! As long as it fails with ASan, that's fine, there are multiple bots building with that enabled. If it would be possible to reduce it even further, that would obviously be great, but it probably needs a large enough input to trigger.

llvm/lib/Transforms/Scalar/SCCP.cpp
536 ↗(On Diff #333401)

nit: perhaps say something along the lines: Copy additional users before notifying them of changes, because new users may be added, potentially invalidating the iterator. (also comments should be full sentences ending with period .)

538 ↗(On Diff #333401)

Please address the lint warning here.

Also, this can probably just be a SmallVector, right?

539 ↗(On Diff #333401)

nit: also no braces should be required here or the next loop.

llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll
3

It would be great if you could add a brief explanation to the test. E.g. say something like: This test checks for an iterator invalidation issue, which only gets exposed on a large-enough test case. We intentionally do not check the output.

848

nit: dso_local and #0 should not be needed (here and elsewhere)

even tinier nit: perhaps it would be good to use a nicer name for the function? Same for the others.

855

That should not be needed.

856

should not be needed

858

should not be needed

860

should not be needed

dim added a comment.Apr 1 2021, 11:20 AM
In D98602#2651521, @dim wrote:

My previous update accidentally dropped the changes to SCCP.cpp.
Squashed both local revisions to get both the functionality change and
the test case.

Thanks for adding the test! As long as it fails with ASan, that's fine, there are multiple bots building with that enabled. If it would be possible to reduce it even further, that would obviously be great, but it probably needs a large enough input to trigger.

This was reduced by bugpoint, which I ran a few times in sequence (using an ASan build), but it couldn't get it smaller. Indeed, you have to 'exercise' the markUsersAsChanged loop enough times to trigger this problem.

Something similar was also the case for the creduce'd test case in D98602#2626988, though that didn't even require ASan on FreeBSD, it simply segfaulted.

llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll
848

nit: dso_local and #0 should not be needed (here and elsewhere)

even tinier nit: perhaps it would be good to use a nicer name for the function? Same for the others.

Yeah, I'll just use the very fancy names f, g, and h :)

855

I forgot what it was, but I remember there is a tool or script somewhere in the tree to 'cleanup' these .ll test cases, stripping off all the unimportant output? Otherwise, I'll just delete these by hand.

fhahn added a comment.Apr 1 2021, 1:42 PM
In D98602#2664740, @dim wrote:
In D98602#2651521, @dim wrote:

My previous update accidentally dropped the changes to SCCP.cpp.
Squashed both local revisions to get both the functionality change and
the test case.

Thanks for adding the test! As long as it fails with ASan, that's fine, there are multiple bots building with that enabled. If it would be possible to reduce it even further, that would obviously be great, but it probably needs a large enough input to trigger.

This was reduced by bugpoint, which I ran a few times in sequence (using an ASan build), but it couldn't get it smaller. Indeed, you have to 'exercise' the markUsersAsChanged loop enough times to trigger this problem.

Sounds good to me, thanks for putting all the effort into getting a reasonable LLVM IR test!

llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll
848

Sounds good to me!

855

I'm not aware of such a tool, sorry!

lebedev.ri added inline comments.Apr 1 2021, 1:43 PM
llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll
855

llvm-reduce?

dim updated this revision to Diff 334966.Apr 2 2021, 8:59 AM

Address review comments.

fhahn added a comment.Apr 2 2021, 9:43 AM
In D98602#2666323, @dim wrote:

Address review comments.

I think that looks good now, but looks like the current review does not have the latest full version of the patch. If you add a new commit and update the review with that patch, the patch will get replaced with the new diff. I think you need to squash the follow-up commits into a single one and then update the review.

dim updated this revision to Diff 334975.Apr 2 2021, 9:48 AM

Squash commits to convince Phabricator.

fhahn accepted this revision.Apr 2 2021, 9:51 AM

LGTM, thanks!

llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll
854

tiniest nit: drop #1.

This revision is now accepted and ready to land.Apr 2 2021, 9:51 AM
This revision was landed with ongoing or failed builds.Apr 2 2021, 10:06 AM
This revision was automatically updated to reflect the committed changes.
dim added inline comments.Apr 2 2021, 10:07 AM
llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll
854

tiniest nit: drop #1.

Thanks, I added this in the actual commit.