Lowered kMaxVirtualRegisters (fixes v8 2139 and chrome 123822 and 128252). (issue 10966031)

13 views
Skip to first unread message

mma...@chromium.org

unread,
Sep 21, 2012, 7:18:27 AM9/21/12
to jkum...@chromium.org, v8-...@googlegroups.com
Reviewers: Jakob,

Description:
Lowered kMaxVirtualRegisters (fixes v8 2139 and chrome 123822 and 128252).

BUG=128252


Please review this at https://chromiumcodereview.appspot.com/10966031/

SVN Base: https://v8.googlecode.com/svn/branches/bleeding_edge

Affected files:
M src/lithium.h


Index: src/lithium.h
diff --git a/src/lithium.h b/src/lithium.h
index
e1cd52aa0acc4e9642c7c3f9ef14646de6290a04..fdc61cb6fef3a316c8555433166a690d99e70a5f
100644
--- a/src/lithium.h
+++ b/src/lithium.h
@@ -153,7 +153,10 @@ class LUnallocated: public LOperand {
kVirtualRegisterWidth> {
};

- static const int kMaxVirtualRegisters = 1 << kVirtualRegisterWidth;
+ // kMaxVirtualRegisters should not exceed 1 << kVirtualRegisterWidth but
we
+ // are setting it to a lower value otherwise crankshaft is too slow.
+ STATIC_ASSERT(kVirtualRegisterWidth > 13);
+ static const int kMaxVirtualRegisters = 1 << 13;
static const int kMaxFixedIndex = 63;
static const int kMinFixedIndex = -64;



veg...@google.com

unread,
Sep 21, 2012, 7:42:08 AM9/21/12
to mma...@chromium.org, jkum...@chromium.org, v8-...@googlegroups.com
dbc

[I would say that big functions should be sent to background thread for
compilation or split into chunks for chunked compilation; but this is too
big a
change]


https://chromiumcodereview.appspot.com/10966031/diff/1/src/lithium.h
File src/lithium.h (right):

https://chromiumcodereview.appspot.com/10966031/diff/1/src/lithium.h#newcode159
src/lithium.h:159: static const int kMaxVirtualRegisters = 1 << 13;
can you give some of this bits to other fields?

if you increase kMax/MinFixedIndex it will help Issue 2223.

https://chromiumcodereview.appspot.com/10966031/

mma...@chromium.org

unread,
Sep 21, 2012, 9:53:21 AM9/21/12
to jkum...@chromium.org, veg...@google.com, v8-...@googlegroups.com
I totally agree on the separate thread approach, apart from the fact that we
also had OOM issues with too many vregs so the limit had to be lowered
anyway
(reaching the OOM condition is painfully slow on most machines).

This last patch settles on 15 bits after testing on many benchmarks trying
to
find a reasonable compromise, and it gives all remaining bits to
kFixedIndexWidth.

https://chromiumcodereview.appspot.com/10966031/

jkum...@chromium.org

unread,
Sep 21, 2012, 9:58:14 AM9/21/12
to mma...@chromium.org, veg...@google.com, v8-...@googlegroups.com
Reply all
Reply to author
Forward
0 new messages