Ensure that multitarget AOT builds have consistent random sequence #7717
Conversation
If a Generator uses random_float() (or the int or uint versions), and is used in a multitarget build, we weren't resetting the counters for random generation between each subtarget... meaning that each subtarget would get a different random sequence, leading to some ery hard-to-debug test failures when running on different hardware variants. This PR ensures that the relevant counters are all reset before each subtarget is generated, so that each should see the same sequence of random number generation.
steven-johnson
changed the title
steven-johnson
added
the
release_notes
|
Do we want to just reset the random counters, or do we want to centralize and reset all state (i.e. unique name counters too)? |
I'd be very much in favor of that. I'm not sure what else to add, though -- unique-name stuff, anything else? (And to instantly add scope-creep -- could we move all such global state into a single struct that is referenced by a thread-local pointer? :-) Having compiler state in random globals has always irked me from a design perspective, but if we could at least collapse it into a single global struct it would be less bad...) |
|
In theory IR can be generated from multiple threads and then used in a single pipeline, so I don't think we can use thread-local state. |
Yeah, I guess that would take some more careful thinking. But moving all the mutable global state into a single struct seems like a worthwhile goal regardless. |
This is a stab at an alternative to #7717, per discussion there. It doesn't necessarily have all the mutable globals moved here, but at least the ones we discussed. Better? Worse? What do you think?
|
To add more motivation for this PR, in libfuzzer, each invocation of the fuzzer API will create the same pipeline but with incrementing suffix like |
This is a stab at an alternative to #7717, per discussion there. It doesn't necessarily have all the mutable globals moved here, but at least the ones we discussed. Better? Worse? What do you think?
|
Closing in favor of #7722 |
|
Reopening this as #7722 ran into some sticky issues, and the original impetus for investigating this was just the random sequences. |
src/IROperator.h
Outdated
| @@ -351,6 +351,11 @@ Expr requirement_failed_error(Expr condition, const std::vector<Expr> &args); | |||
|
|
|||
| Expr memoize_tag_helper(Expr result, const std::vector<Expr> &cache_key_values); | |||
|
|
|||
| /** Reset the counters used for random-number seeds in random_float/int/uint | |||
| * when no seed is explicitly passed in; this is used for multitarget compilation | |||
There was a problem hiding this comment.
The counter is added even if a seed is passed in. The seed is additional.
| } // namespace | ||
|
|
||
| // A counter to use in tagging random variables. | ||
| // Note that this will be reset by Internal::reset_random_counters(). | ||
| std::atomic<int> random_variable_counter = 0; |
There was a problem hiding this comment.
The only effect of this tag seems to be to give each compiled Pipeline a separate set of random numbers, in addition to each individual call to random_whatever being distinct. resetting the random counters in Module.cpp below seems like it would just totally undo this...
So if this counter is important, we'd better figure out why, because this PR breaks it.
There was a problem hiding this comment.
Disregard, this is a per-Func counter, not a per-Pipeline counter.
There was a problem hiding this comment.
So IIUC you are saying this is fine as-is in this PR.
|
PTAL |
|
Clean aside from the since-fixed bogus clang-tidy failure, PTAL |
…alide#7717) * Fix CMake test for generator_aot_multitarget * Ensure that multitarget AOT builds have consistent random numbers If a Generator uses random_float() (or the int or uint versions), and is used in a multitarget build, we weren't resetting the counters for random generation between each subtarget... meaning that each subtarget would get a different random sequence, leading to some ery hard-to-debug test failures when running on different hardware variants. This PR ensures that the relevant counters are all reset before each subtarget is generated, so that each should see the same sequence of random number generation. * Update CMakeLists.txt * Update multitarget_aottest.cpp * Combine float/uint counters
If a Generator uses random_float() (or the int or uint versions), and is used in a multitarget build, we weren't resetting the counters for random generation between each subtarget... meaning that each subtarget would get a different random sequence, leading to some ery hard-to-debug test failures when running on different hardware variants. This PR ensures that the relevant counters are all reset before each subtarget is generated, so that each should see the same sequence of random number generation.