Use separate random number generator for each randomization pass

Use separate random number generator for each randomization pass

This removes random number generator from GlobalContext class and decouples different randomization passes

1. Add a new constructor for random number generator which merge three arguments to into one seed for the underlying implementation of random number generator.

RandomNumberGenerator(uint64_t Seed, RandomizationPassesEnum RandomizationPassID, uint64_t Salt=0)

param Seed: Should be the global random number seed passed through command line.
param RandomizationPassID: Should be the ID for different randomization passes.
param Salt: Should be an additional integer salt, default to be 0.

2. Move the creation of random number generators to the call sites of randomization passes. Each randomization pass create its own random number generator with specific salt value.

Function reordering:		Salt = 0 (default)
Basic Block reordering:		Salt = Function Sequence Number
Global Variable reordering:	Salt = 0 (default)
Pooled Constants reordering:	Salt = Constants' Kind value (return of getKind())
               *Jump Tables:	Salt = 0
Nop Insertion:			Salt = Function Sequence Number
Register Alloc Randomization:	Salt = (Function Sequence Number << 1) ^ (Kind == RAK_Phi ? 0u : 1u)
Constants Blinding:		Salt = Function Sequence Number

*Jump tables are treated as pooled constants, but without Kind value as salt.

BUG=
R=stichnot@chromium.org

Committed: https://gerrit.chromium.org/gerrit/gitweb?p=native_client/pnacl-subzero.git;a=commit;h=aee5fa8dd6aa948160a290c8237d7ae4875811fb

[qining, 2015-08-18 23:47:38]

qining@google.com changed reviewers:
+ jpp@chromium.org, jvoung@chromium.org, stichnot@chromium.org

[Jim Stichnoth, 2015-08-19 19:03:05]

https://codereview.chromium.org/1300993002/diff/20001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceCfg.h#newcode252
src/IceCfg.h:252: uint32_t ConstantBlindingCookie = 0; /// Cookie for Constant
Blinding
Make capitalization consistent across the adjacent comments.

https://codereview.chromium.org/1300993002/diff/20001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceDefs.h#newcode280
src/IceDefs.h:280: RS_BasicBlockReordering,
Usually the enum value prefix is directly derived from the enum type name, so
here it might be RPE_ .

https://codereview.chromium.org/1300993002/diff/20001/src/IceDefs.h#newcode287
src/IceDefs.h:287: RS_END = 16
Instead, use
  RPE_num

The _num name is used in lots of enums in the code base.

And there's no reason to specify 16 here.

https://codereview.chromium.org/1300993002/diff/20001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:516: if (!Pool.empty()) {
Can combine this and the above condition, as noted in another comment.

https://codereview.chromium.org/1300993002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:881: // Create a random number generator for jump
tables reordering, considering
Either reformat this entire comment (to fix weird line break), or add a blank
comment line.

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp
File src/IceRNG.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp#newcode33
src/IceRNG.cpp:33: /// Create a random number generator with: global seed,
The same comment/docstring appears in IceRNG.h and IceRNG.cpp.  I think you can
just put it in one place.

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp#newcode38
src/IceRNG.cpp:38: /// Final seed = Seed ^ Salt << 16 ^ RandomizationPassID <<
28
I'd like to see an explanation of these magic shift constants.  (Also, when we
discussed this, I was assuming the final seed was 32 bits, but it's actually 64
bits, so the shift constants should change.)  Here's a draft:

The global seed is 64 bits; since it is likely to originate from the system
time, the lower bits are more "valuable" than the upper bits.  As such, we merge
the pass ID and the salt into the global seed by xor'ing them into high bit
ranges.  We expect the pass ID to fit within 4 bits, so it gets shifted by 60 to
merge into the upper 4 bits.  We expect the salt (generaly the function sequence
number) to fit within 12 bits, so it gets shifted by 48 before merging.

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp#newcode41
src/IceRNG.cpp:41: : State(Seed ^ Salt << 16 ^ RandomizationPassID << 28) {
Instead of 16 and 28 (or 48 and 60 in the proposed changes), I would use
something like:
  constexpr NumBitsGlobalSeed = CHAR_BIT * sizeof(State);
  constexpr NumBitsPassID = 4;
  constexpr NumBitsSalt = 12;
  State = Seed ^
          (RandomizationPassID << (NumBitsGlobalSeed - NumBitsPassId)) ^
          (Salt << (NumBitsGlobalSeed - NumBitsPassID - NumBitsSalt));

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp#newcode42
src/IceRNG.cpp:42: assert(RandomizationPassID < 0x10 && "RandomizationPassID
requires more than "
Use a static_assert instead:

  static_assert(blah < (1 << NumBitsPassID), "NumBitsPassID too small");

That said, this may eventually have to go back to a regular assert, if we need
to allow the target lowering to add new pass IDs.

https://codereview.chromium.org/1300993002/diff/20001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:779: Func->getTarget()->makeRandomRegisterPermutation(
Can you make it so that the RNG is initialized using a different passID when
Kind==RAK_Phi?  (This means having two separate randomization pass IDs for
register allocation.)  The issue is that register allocation is called twice for
O2, once before and once after phi lowering.

https://codereview.chromium.org/1300993002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:735: if (!Pool.empty()) {
can probably combine this condition with the above:
  if (shouldReorder && !empty()) { ... }

https://codereview.chromium.org/1300993002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-basic-blocks.ll (right):

https://codereview.chromium.org/1300993002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-basic-blocks.ll:8: ; RUN: %p2i -i %s
--filetype=asm --args -O2 -sz-seed=3 \
Why did the seed have to change?  I assume it was because seed=1 and seed=2
produced identical results for this small test?

[qining, 2015-08-19 23:25:49]

The tests still need to be fixed, will upload them later.

https://codereview.chromium.org/1300993002/diff/20001/src/IceCfg.h
File src/IceCfg.h (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceCfg.h#newcode252
src/IceCfg.h:252: uint32_t ConstantBlindingCookie = 0; /// Cookie for Constant
Blinding
On 2015/08/19 19:03:04, stichnot wrote:
> Make capitalization consistent across the adjacent comments.

Done.

https://codereview.chromium.org/1300993002/diff/20001/src/IceDefs.h
File src/IceDefs.h (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceDefs.h#newcode280
src/IceDefs.h:280: RS_BasicBlockReordering,
On 2015/08/19 19:03:04, stichnot wrote:
> Usually the enum value prefix is directly derived from the enum type name, so
> here it might be RPE_ .

Done.

https://codereview.chromium.org/1300993002/diff/20001/src/IceDefs.h#newcode287
src/IceDefs.h:287: RS_END = 16
On 2015/08/19 19:03:04, stichnot wrote:
> Instead, use
>   RPE_num
> 
> The _num name is used in lots of enums in the code base.
> 
> And there's no reason to specify 16 here.

Done.

https://codereview.chromium.org/1300993002/diff/20001/src/IceELFObjectWriter.cpp
File src/IceELFObjectWriter.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceELFObjectWriter....
src/IceELFObjectWriter.cpp:516: if (!Pool.empty()) {
On 2015/08/19 19:03:04, stichnot wrote:
> Can combine this and the above condition, as noted in another comment.

Done.

https://codereview.chromium.org/1300993002/diff/20001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:881: // Create a random number generator for jump
tables reordering, considering
On 2015/08/19 19:03:04, stichnot wrote:
> Either reformat this entire comment (to fix weird line break), or add a blank
> comment line.

Done.

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp
File src/IceRNG.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp#newcode33
src/IceRNG.cpp:33: /// Create a random number generator with: global seed,
On 2015/08/19 19:03:04, stichnot wrote:
> The same comment/docstring appears in IceRNG.h and IceRNG.cpp.  I think you
can
> just put it in one place.

Done. Removed the one in .cpp.

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp#newcode38
src/IceRNG.cpp:38: /// Final seed = Seed ^ Salt << 16 ^ RandomizationPassID <<
28
On 2015/08/19 19:03:04, stichnot wrote:
> I'd like to see an explanation of these magic shift constants.  (Also, when we
> discussed this, I was assuming the final seed was 32 bits, but it's actually
64
> bits, so the shift constants should change.)  Here's a draft:
> 
> The global seed is 64 bits; since it is likely to originate from the system
> time, the lower bits are more "valuable" than the upper bits.  As such, we
merge
> the pass ID and the salt into the global seed by xor'ing them into high bit
> ranges.  We expect the pass ID to fit within 4 bits, so it gets shifted by 60
to
> merge into the upper 4 bits.  We expect the salt (generaly the function
sequence
> number) to fit within 12 bits, so it gets shifted by 48 before merging.

Done.

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp#newcode41
src/IceRNG.cpp:41: : State(Seed ^ Salt << 16 ^ RandomizationPassID << 28) {
On 2015/08/19 19:03:04, stichnot wrote:
> Instead of 16 and 28 (or 48 and 60 in the proposed changes), I would use
> something like:
>   constexpr NumBitsGlobalSeed = CHAR_BIT * sizeof(State);
>   constexpr NumBitsPassID = 4;
>   constexpr NumBitsSalt = 12;
>   State = Seed ^
>           (RandomizationPassID << (NumBitsGlobalSeed - NumBitsPassId)) ^
>           (Salt << (NumBitsGlobalSeed - NumBitsPassID - NumBitsSalt));

It seems I have to put these constexpr and the assignment of State in the
constructor of RNG. Otherwise I have to declare them static constexpr in order
to access sizeof(State).

https://codereview.chromium.org/1300993002/diff/20001/src/IceRNG.cpp#newcode42
src/IceRNG.cpp:42: assert(RandomizationPassID < 0x10 && "RandomizationPassID
requires more than "
On 2015/08/19 19:03:04, stichnot wrote:
> Use a static_assert instead:
> 
>   static_assert(blah < (1 << NumBitsPassID), "NumBitsPassID too small");
> 
> That said, this may eventually have to go back to a regular assert, if we need
> to allow the target lowering to add new pass IDs.

Done.

https://codereview.chromium.org/1300993002/diff/20001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:779: Func->getTarget()->makeRandomRegisterPermutation(
On 2015/08/19 19:03:04, stichnot wrote:
> Can you make it so that the RNG is initialized using a different passID when
> Kind==RAK_Phi?  (This means having two separate randomization pass IDs for
> register allocation.)  The issue is that register allocation is called twice
for
> O2, once before and once after phi lowering.

Done. The salt will be calculated here and then passed down to
makeRandomRegisterPermutation() to create RNG.

https://codereview.chromium.org/1300993002/diff/20001/src/IceTargetLoweringX8...
File src/IceTargetLoweringX8632.cpp (right):

https://codereview.chromium.org/1300993002/diff/20001/src/IceTargetLoweringX8...
src/IceTargetLoweringX8632.cpp:735: if (!Pool.empty()) {
On 2015/08/19 19:03:04, stichnot wrote:
> can probably combine this condition with the above:
>   if (shouldReorder && !empty()) { ... }

Done.

https://codereview.chromium.org/1300993002/diff/20001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-basic-blocks.ll (right):

https://codereview.chromium.org/1300993002/diff/20001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-basic-blocks.ll:8: ; RUN: %p2i -i %s
--filetype=asm --args -O2 -sz-seed=3 \
On 2015/08/19 19:03:04, stichnot wrote:
> Why did the seed have to change?  I assume it was because seed=1 and seed=2
> produced identical results for this small test?

Done. Actually the result of seed=2 is: entry->1->2->3->4. I've changed back to
seed=2, as 3 looks really weird.

[Jim Stichnoth, 2015-08-20 04:04:41]

https://codereview.chromium.org/1300993002/diff/40001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1300993002/diff/40001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:780: // Use function's sequence and Kind value as the salt.
Mention that O2 calls regalloc twice, the second time with RAK_Phi, so we make
sure the Salt is different.

(Also, reformat the comment block to the full 80 columns.)

https://codereview.chromium.org/1300993002/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (left):

https://codereview.chromium.org/1300993002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:226: RNG(Flags.getRandomSeed()), ObjectWriter(),
A few points regarding ClFlags::RandomSeed.  (These could probably be addressed
in a separate CL.)

1. We do need some way to set RandomSeed to something like time(0).  Maybe a
special value like -sz-seed=0 or -sz-seed=-1 would do that.

2. For the browser build of Subzero, we would probably make time(0) the default.
 We definitely don't want to add more flags to the browser interface.

3. When setting the seed to time(0), we need to make it possible to discover
what that value of time(0) was, so that we can debug and reproduce failures:

(a) Print it to StrDump.

(b) Add it to the global initializer list as a hidden/internal symbol.

(c) Get it logged somehow in the browser build.

Jan, do you have any thoughts on this, especially the browser side?

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/rng.ll (right):

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/rng.ll:16: ; RUN:    -nop-insertion=false \
For consistency with other .ll files, probably use 0 instead of false.  (here
and below)

[jvoung (off chromium), 2015-08-20 12:41:04]

https://codereview.chromium.org/1300993002/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (left):

https://codereview.chromium.org/1300993002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:226: RNG(Flags.getRandomSeed()), ObjectWriter(),
On 2015/08/20 04:04:41, stichnot wrote:
> A few points regarding ClFlags::RandomSeed.  (These could probably be
addressed
> in a separate CL.)
> 
> 1. We do need some way to set RandomSeed to something like time(0).  Maybe a
> special value like -sz-seed=0 or -sz-seed=-1 would do that.
> 
> 2. For the browser build of Subzero, we would probably make time(0) the
default.
>  We definitely don't want to add more flags to the browser interface.

I need to look at the rest of the CL, but BrowserCompileServer::getParsedFlags
initializes the ClFlags specially for the browser (e.g., default to --sandbox,
and ELF output). That could be a place to initialize the seed without needing to
plumb anything from the chrome renderer.

> 
> 3. When setting the seed to time(0), we need to make it possible to discover
> what that value of time(0) was, so that we can debug and reproduce failures:
> 
> (a) Print it to StrDump.
> 
> (b) Add it to the global initializer list as a hidden/internal symbol.
> 
> (c) Get it logged somehow in the browser build.
> 
> Jan, do you have any thoughts on this, especially the browser side?

Hmm, at least if you run chrome from the terminal it should show up in the
terminal
(https://developer.chrome.com/native-client/devguide/devcycle/debugging#using-...).
So, (a) could then help developers... No user would run chrome that way though
=).  Unfortunately we don't have a nicer way to pipe stdout to the JS console
automatically (I think Mark advocated for that, but there was some pushback
about feature creep from the pepper folks a while back...).

For (b), you mean so that someone can look in the browser cache for the
resulting nexe, and attach that to a bug report or if there is automatic crash
reporting which attached parts of the nexe? Then we can inspect that nexe (parts
of it)? That could be interesting. You might also be able to do something with
ELF like add a NOTE section, kind of like .note.gnu.build-id, but
.note.sz.random-seed or something.

For (c), yeah... (a) is about the closest you can get. You could add something
to the private IRT interface for just subzero, and plumb that out to the browser
after compilation finishes, then push that to the JS console or wherever you
like. We currently only plumb crash messages from StrError and
report_fatal_error(), but that is only if Subzero itself crashes, not if the app
crashes.

[jvoung (off chromium), 2015-08-20 13:43:30]

random nits =)

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-global-variables.ll (right):

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:57: ;@__init_array_start =
internal constant [0 x i8] zeroinitializer, align 4
can we remove the commented out bits?

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/rng.ll (right):

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/rng.ll:21: ; RUN: %if --need=target_X8632 --command
%p2i -i %s \
This appears to be the only command  that does "%if --need=target_X8632 ...".

If they all rely on X8632 maybe just put it in REQUIRES: ... (not sure the
syntax for multiple requirements).

Or make a randomization-related subdirectory where the lit.local.cfg checks for
allow_dump (e.g.,
toolchain_build/src/subzero/tests_lit/reader_tests/lit.local.cfg), to avoid
having multiple REQUIRES for this particular test. Not all of the randomization
tests require allow_dump though...

[Jim Stichnoth, 2015-08-20 15:14:26]

Two other comments:

1. Please edit the Subject to be 80 characters or fewer.

2. Can you verfify that spec2k/x8632 runs correctly with all randomizations
turned on, for Om1 and O2, and for sandbox and native?

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/rng.ll (right):

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/rng.ll:21: ; RUN: %if --need=target_X8632 --command
%p2i -i %s \
On 2015/08/20 13:43:30, jvoung wrote:
> This appears to be the only command  that does "%if --need=target_X8632 ...".
> 
> If they all rely on X8632 maybe just put it in REQUIRES: ... (not sure the
> syntax for multiple requirements).
> 
> Or make a randomization-related subdirectory where the lit.local.cfg checks
for
> allow_dump (e.g.,
> toolchain_build/src/subzero/tests_lit/reader_tests/lit.local.cfg), to avoid
> having multiple REQUIRES for this particular test. Not all of the
randomization
> tests require allow_dump though...

Yeah, it seems to me that at least for now, require x8632 and add -target=x8632
to all the RUN commands.

In the future, I suppose we might end up with some non-x86 target-specific pass
that allows randomization, but it doesn't necessarily need to be tested in this
file.

[qining, 2015-08-20 16:19:34]

Updated with Patch set 6.

https://codereview.chromium.org/1300993002/diff/40001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1300993002/diff/40001/src/IceRegAlloc.cpp#new...
src/IceRegAlloc.cpp:780: // Use function's sequence and Kind value as the salt.
On 2015/08/20 04:04:41, stichnot wrote:
> Mention that O2 calls regalloc twice, the second time with RAK_Phi, so we make
> sure the Salt is different.
> 
> (Also, reformat the comment block to the full 80 columns.)

Done.

https://codereview.chromium.org/1300993002/diff/80001/src/IceGlobalContext.cpp
File src/IceGlobalContext.cpp (left):

https://codereview.chromium.org/1300993002/diff/80001/src/IceGlobalContext.cp...
src/IceGlobalContext.cpp:226: RNG(Flags.getRandomSeed()), ObjectWriter(),
On 2015/08/20 12:41:04, jvoung wrote:
> On 2015/08/20 04:04:41, stichnot wrote:
> > A few points regarding ClFlags::RandomSeed.  (These could probably be
> addressed
> > in a separate CL.)
> > 
> > 1. We do need some way to set RandomSeed to something like time(0).  Maybe a
> > special value like -sz-seed=0 or -sz-seed=-1 would do that.
> > 
> > 2. For the browser build of Subzero, we would probably make time(0) the
> default.
> >  We definitely don't want to add more flags to the browser interface.
> 
> I need to look at the rest of the CL, but BrowserCompileServer::getParsedFlags
> initializes the ClFlags specially for the browser (e.g., default to --sandbox,
> and ELF output). That could be a place to initialize the seed without needing
to
> plumb anything from the chrome renderer.
> 
> > 
> > 3. When setting the seed to time(0), we need to make it possible to discover
> > what that value of time(0) was, so that we can debug and reproduce failures:
> > 
> > (a) Print it to StrDump.
> > 
> > (b) Add it to the global initializer list as a hidden/internal symbol.
> > 
> > (c) Get it logged somehow in the browser build.
> > 
> > Jan, do you have any thoughts on this, especially the browser side?
> 
> Hmm, at least if you run chrome from the terminal it should show up in the
> terminal
>
(https://developer.chrome.com/native-client/devguide/devcycle/debugging#using-...).
> So, (a) could then help developers... No user would run chrome that way though
> =).  Unfortunately we don't have a nicer way to pipe stdout to the JS console
> automatically (I think Mark advocated for that, but there was some pushback
> about feature creep from the pepper folks a while back...).
> 
> For (b), you mean so that someone can look in the browser cache for the
> resulting nexe, and attach that to a bug report or if there is automatic crash
> reporting which attached parts of the nexe? Then we can inspect that nexe
(parts
> of it)? That could be interesting. You might also be able to do something with
> ELF like add a NOTE section, kind of like .note.gnu.build-id, but
> .note.sz.random-seed or something.
> 
> For (c), yeah... (a) is about the closest you can get. You could add something
> to the private IRT interface for just subzero, and plumb that out to the
browser
> after compilation finishes, then push that to the JS console or wherever you
> like. We currently only plumb crash messages from StrError and
> report_fatal_error(), but that is only if Subzero itself crashes, not if the
app
> crashes.

I guess this would be a separate CL?

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/reorder-global-variables.ll (right):

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/reorder-global-variables.ll:57: ;@__init_array_start =
internal constant [0 x i8] zeroinitializer, align 4
On 2015/08/20 13:43:30, jvoung wrote:
> can we remove the commented out bits?

Done.

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
File tests_lit/llvm2ice_tests/rng.ll (right):

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/rng.ll:16: ; RUN:    -nop-insertion=false \
On 2015/08/20 04:04:41, stichnot wrote:
> For consistency with other .ll files, probably use 0 instead of false.  (here
> and below)

Done.

https://codereview.chromium.org/1300993002/diff/80001/tests_lit/llvm2ice_test...
tests_lit/llvm2ice_tests/rng.ll:21: ; RUN: %if --need=target_X8632 --command
%p2i -i %s \
On 2015/08/20 15:14:26, stichnot wrote:
> On 2015/08/20 13:43:30, jvoung wrote:
> > This appears to be the only command  that does "%if --need=target_X8632
...".
> > 
> > If they all rely on X8632 maybe just put it in REQUIRES: ... (not sure the
> > syntax for multiple requirements).
> > 
> > Or make a randomization-related subdirectory where the lit.local.cfg checks
> for
> > allow_dump (e.g.,
> > toolchain_build/src/subzero/tests_lit/reader_tests/lit.local.cfg), to avoid
> > having multiple REQUIRES for this particular test. Not all of the
> randomization
> > tests require allow_dump though...
> 
> Yeah, it seems to me that at least for now, require x8632 and add
-target=x8632
> to all the RUN commands.
> 
> In the future, I suppose we might end up with some non-x86 target-specific
pass
> that allows randomization, but it doesn't necessarily need to be tested in
this
> file.

Done. Add:
REQUIRES: allow_dump, target_X8632.
--target x8632

[Jim Stichnoth, 2015-08-20 16:23:43]

Sorry to nitpick, but may I suggest, "Use a separate random number generator for
each randomization pass".

Also, make sure the subject and the first line of the description are the same,
as I'm not entirely sure which is used as the final git commit message.

[native-client-reviews_googlegroups.com, 2015-08-20 16:25:22]

Got it.

On Thu, Aug 20, 2015 at 9:23 AM, <stichnot@chromium.org> wrote:

> Sorry to nitpick, but may I suggest, "Use a separate random number
> generator for
> each randomization pass".
>
> Also, make sure the subject and the first line of the description are the
> same,
> as I'm not entirely sure which is used as the final git commit message.
>
> https://codereview.chromium.org/1300993002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[native-client-reviews_googlegroups.com, 2015-08-20 17:20:27]

About testing of SPEC2K with all randomization passes turned on, I've
tested O2 with and without sandbox and all benchmarks run fine. Currently
testing Om1 with sandboxing.

Thanks,

On Thu, Aug 20, 2015 at 9:25 AM, Qining Lu <qining@google.com> wrote:

> Got it.
>
> On Thu, Aug 20, 2015 at 9:23 AM, <stichnot@chromium.org> wrote:
>
>> Sorry to nitpick, but may I suggest, "Use a separate random number
>> generator for
>> each randomization pass".
>>
>> Also, make sure the subject and the first line of the description are the
>> same,
>> as I'm not entirely sure which is used as the final git commit message.
>>
>> https://codereview.chromium.org/1300993002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews.
For more options, visit https://groups.google.com/d/optout.

[Jim Stichnoth, 2015-08-20 20:01:33]

lgtm

https://codereview.chromium.org/1300993002/diff/100001/src/IceRegAlloc.cpp
File src/IceRegAlloc.cpp (right):

https://codereview.chromium.org/1300993002/diff/100001/src/IceRegAlloc.cpp#ne...
src/IceRegAlloc.cpp:780: // function's sequence and Kind value as the Salt.
Because regalloc()
regAlloc

https://codereview.chromium.org/1300993002/diff/100001/src/IceRegAlloc.cpp#ne...
src/IceRegAlloc.cpp:781: // are called twice under O2, the second time with
RAK_Phi, we check
is called

https://codereview.chromium.org/1300993002/diff/100001/src/IceRegAlloc.cpp#ne...
src/IceRegAlloc.cpp:782: // Kind == RAK_Phi to determine the least bit to make
sure the Salt is
I would say "lowest-order" instead of "least".

[qining, 2015-08-20 21:59:07]

Committed patchset #8 (id:140001) manually as
aee5fa8dd6aa948160a290c8237d7ae4875811fb (presubmit successful).