[PATCH] D98602: [SCCP] Avoid modifying AdditionalUsers while iterating over it

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D98602#2651521 <https://reviews.llvm.org/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.



================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:536
     if (Iter != AdditionalUsers.end()) {
-      for (User *U : Iter->second)
-        if (auto *UI = dyn_cast<Instruction>(U))
-          OperandChangedState(UI);
+      // Cache Instructions to update and update later
+      // in order to avoid iterator invalidation
----------------
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 .)


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:538
+      // in order to avoid iterator invalidation
+      SmallPtrSet<Instruction *, 2> toNotify;
+      for (User *U : Iter->second) {
----------------
Please address the lint warning here.

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


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:539
+      SmallPtrSet<Instruction *, 2> toNotify;
+      for (User *U : Iter->second) {
+        if (auto *UI = dyn_cast<Instruction>(U)) {
----------------
nit: also no braces should be required here or the next loop.


================
Comment at: llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll:2
+; RUN: opt < %s -ipsccp -disable-output
+; PR49582
+
----------------
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.` 


================
Comment at: llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll:847
+
+declare dso_local i32 @_Z1biii() #0
+
----------------
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.


================
Comment at: llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll:854
+
+attributes #0 = { "tune-cpu"="generic" }
+attributes #1 = { nofree nosync nounwind willreturn }
----------------
That should not be needed.


================
Comment at: llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll:855
+attributes #0 = { "tune-cpu"="generic" }
+attributes #1 = { nofree nosync nounwind willreturn }
+
----------------
should not be needed


================
Comment at: llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll:857
+
+!llvm.ident = !{!0}
+
----------------
should not be needed


================
Comment at: llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll:859
+
+!0 = !{!"clang version 13.0.0 (https://github.com/llvm/llvm-project.git 29d46760599b32504f4dfda4de69aca8b9f1f65e)"}
----------------
should not be needed


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98602/new/

https://reviews.llvm.org/D98602