Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(217)

Issue 10942: Start IA32 implemenetation of regexp2k (Closed)

Created:
12 years, 1 month ago by Lasse Reichstein
Modified:
9 years, 6 months ago
Reviewers:
Erik Corry
CC:
v8-dev_googlecode.com
Visibility:
Public.

Description

* Create ByteArray to hold constants outside of the code. Can reuse the same ByteArray for several bytes. * Start on regexp-macro-assembler-ia32.

Patch Set 1 #

Patch Set 2 : Addressed review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+777 lines, -10 lines) Patch
M regexp2000/src/assembler-ia32.h View 5 chunks +8 lines, -1 line 0 comments Download
M regexp2000/src/assembler-ia32.cc View 4 chunks +42 lines, -7 lines 0 comments Download
M regexp2000/src/assembler-ia32-inl.h View 1 1 chunk +8 lines, -0 lines 0 comments Download
M regexp2000/src/heap.h View 1 chunk +7 lines, -1 line 0 comments Download
M regexp2000/src/heap.cc View 1 chunk +18 lines, -0 lines 0 comments Download
M regexp2000/src/jsregexp.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M regexp2000/src/regexp-macro-assembler.h View 2 chunks +40 lines, -1 line 0 comments Download
A regexp2000/src/regexp-macro-assembler.cc View 1 chunk +62 lines, -0 lines 0 comments Download
A regexp2000/src/regexp-macro-assembler-ia32.h View 1 1 chunk +151 lines, -0 lines 0 comments Download
A regexp2000/src/regexp-macro-assembler-ia32.cc View 1 1 chunk +437 lines, -0 lines 0 comments Download

Messages

Total messages: 1 (0 generated)
Erik Corry
12 years, 1 month ago (2008-11-14 10:38:58 UTC) #1
LGTM

http://codereview.chromium.org/10942/diff/1/2
File regexp2000/src/assembler-ia32-inl.h (right):

http://codereview.chromium.org/10942/diff/1/2#newcode209
Line 209: if (x.rmode_ != RelocInfo::NONE) RecordRelocInfo(x.rmode_);
Perhaps we should just assert that the mode is NONE here. After all, I don't
think there's a reloc mode that can cope with 16 bit values.

http://codereview.chromium.org/10942/diff/1/8
File regexp2000/src/regexp-macro-assembler-ia32.cc (right):

http://codereview.chromium.org/10942/diff/1/8#newcode40
Line 40: *         great.
The bytecode backend always cheks for end of input immediately when loading a
character into the current character 'register'.  It branches immediately so
there is no need for a special value.  I think it is possible and cleaner to do
the ia32 backend in the same way.

http://codereview.chromium.org/10942/diff/1/8#newcode61
Line 61: * The data before ebp must be placed there by the calling code.
I think in the long run we may want to put the registers somewhere else.

http://codereview.chromium.org/10942/diff/1/8#newcode78
Line 78: void RegExpMacroAssemblerIA32::AdvanceCurrentPosition(int by) {
Sometimes we want to advance the current position without loading the current
character.  I think you should move the current character loading instructions
to the places where it is used.  Or we should expose the load-current-character
instruction in the macro assembler interface.  It's fine to save this change for
another changeset.

http://codereview.chromium.org/10942/diff/1/8#newcode118
Line 118: }
I think this is the wrong place for this optimization.  When we specialize the
upper layers for subject character width we will be able to assume that
CheckBitmap is never even called under the circumstances that are being
optimized for here.

http://codereview.chromium.org/10942/diff/1/8#newcode125
Line 125: //__ mov(ecx, position_of_bitmap);  // TODO: Where is the bitmap
stored?
This won't lint.

http://codereview.chromium.org/10942/diff/1/8#newcode274
Line 274: }
Wrong place for optimization.

http://codereview.chromium.org/10942/diff/1/9
File regexp2000/src/regexp-macro-assembler-ia32.h (right):

http://codereview.chromium.org/10942/diff/1/9#newcode28
Line 28: #ifndef REGEXPMACROASSEMBLERIA32_H_
Please insert underscores to make this readable.

Powered by Google App Engine
This is Rietveld 408576698