This is an archive of the discontinued LLVM Phabricator instance.

[Float2Int] Make iteration over Roots deterministic
ClosedPublic

Authored by bjope on Feb 13 2020, 1:55 AM.

Details

Summary

Use a SmallSetVector instead of a SmallPtrSet when collecting
and storing Roots.

The iteration order for a SmallPtrSet is not deterministic,
so in the past the order of items inserted in the WorkList
inside walkBackwards has been non-deterministic. This patch
intends to make the order of rewrites done in Float2Int
deterministic by changing the container for the Roots set.

The semantics result of the transformation should not be
any different afaict. But at least naming of IR variables
(when outputting the result as an ll file) should be more
stable now.

Diff Detail

Event Timeline

bjope created this revision.Feb 13 2020, 1:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2020, 1:55 AM
Herald added a subscriber: mgrang. · View Herald Transcript

(not sure whom to add as reviewer to patches like this, added some random people who has contributed to this pass earlier)

Also see https://reviews.llvm.org/D74533 for a preparatory cleanup patch.

bjope updated this revision to Diff 256725.Apr 10 2020, 5:52 PM

Removed the need for D74533 as pre-commit, by removing the passing of
the Roots member variable as argument to member functions.

spatel added inline comments.Apr 13 2020, 5:56 AM
llvm/lib/Transforms/Scalar/Float2Int.cpp
527–529 ↗(On Diff #256725)

Isn't the change use the member variable directly (rather than passing a param to it) truly NFC?
IIUC, we should make that change first (no need for pre-commit review).

bjope updated this revision to Diff 258049.Apr 16 2020, 7:19 AM

Rebased after pre-commit of the cleanup (we no longer pass around references to Roots).

I'd say we reduced this as much as possible. :)
LGTM

spatel accepted this revision.Apr 17 2020, 6:06 AM
This revision is now accepted and ready to land.Apr 17 2020, 6:06 AM
This revision was automatically updated to reflect the committed changes.