Page MenuHomePhabricator

[LoopVersioning] Don't modify the list that we iterate over in addPHINodes
ClosedPublic

Authored by bjope on May 21 2018, 4:05 AM.

Details

Summary

In LoopVersioning::addPHINodes we need to iterate over all
users for a value "Inst", and if the user is outside of the
VersionedLoop we should replace the use of "Inst" by using
the value "PN" instead.

Replacing the use of "Inst" for a user of "Inst" also means
that Inst->users() is modified. So it is not safe to do the
replace while iterating over Inst->users() as we used to do.
This patch splits the task into two steps. First we iterate
over Inst->users() to find all users that should be updated.
Those users are saved into a local data structure on the stack.
And then, in the second step, we do the actual updates. This
time iterating over the local data structure.

Diff Detail

Repository
rL LLVM

Event Timeline

bjope created this revision.May 21 2018, 4:05 AM
mzolotukhin accepted this revision.May 21 2018, 12:05 PM

Looks good to me! (+ a couple of nitpicks inline)

lib/Transforms/Utils/LoopVersioning.cpp
143–144 ↗(On Diff #147762)

I suggest s/UsersToModify/UsersToUpdate/ and remove the comments as the code is pretty clear and self-explanatory.

145 ↗(On Diff #147762)

How about renaming variable User to U? It's shorter and the new variable name won't overlap with the class name (which is not an error, but just a minor inconvenience when one searches for the variable name in the file, for instance).

This revision is now accepted and ready to land.May 21 2018, 12:05 PM
bjope updated this revision to Diff 147854.May 21 2018, 1:24 PM

Updated according to suggestions from @mzolotukhin.
Thanks for the review (I'll push this tomorrow when I can observe the build bots).

This revision was automatically updated to reflect the committed changes.