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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

In D98602#2664740 <https://reviews.llvm.org/D98602#2664740>, @dim wrote:

> In D98602#2663871 <https://reviews.llvm.org/D98602#2663871>, @fhahn wrote:
>
>> 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.
>
> 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!



================
Comment at: llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll:847
+
+declare dso_local i32 @_Z1biii() #0
+
----------------
dim wrote:
> fhahn wrote:
> > 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.
> > 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` :)
Sounds good to me!


================
Comment at: llvm/test/Transforms/SCCP/pr49582-iterator-invalidation.ll:854
+
+attributes #0 = { "tune-cpu"="generic" }
+attributes #1 = { nofree nosync nounwind willreturn }
----------------
dim wrote:
> fhahn wrote:
> > That should not be needed.
> 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.
> 
I'm not aware of such a tool, sorry!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98602