Refactor FrameAndConstantPoolScope and ConstantPoolUnavailableScope to be architecture independent

Refactor FrameAndConstantPoolScope and ConstantPoolUnavailableScope to be architecture independent

Move the FrameAndConstantPoolScope and ConstantPoolUnavailableScope out of the arm architecture directory to enable them to be used on all architectures.

R=rmcilroy@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24565

[baixo, 2014-09-26 15:38:10]

baixo@chromium.org changed reviewers:
+ rmcilroy@chromium.org

[baixo, 2014-09-26 15:38:10]

Please take a look.

[rmcilroy, 2014-09-26 15:54:58]

https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:1102: // Support not implemented in ia32 yet.
We will probably never us a constant pool on ia32, so you can just remove the
comment and UNREACHABLE and always return false here.

https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:1115: bool is_const_pool_blocked() const {
You don't need this function (that is only for the inline constant pool and will
be going away on Arm once we switch to OOL constant pool).

https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:1122: // Support not implemented in ia32 yet.
replace comment with: "// Constant pool is not supported on ia32"

https://codereview.chromium.org/609843002/diff/1/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/609843002/diff/1/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:902: bool load_constant_pool) {
Add an ASSERT that load_constant_pool is always false.  Also could you rename
the variable (both here and on arm) to load_constant_pool_pointer_reg please.

https://codereview.chromium.org/609843002/diff/1/src/macro-assembler.h
File src/macro-assembler.h (right):

https://codereview.chromium.org/609843002/diff/1/src/macro-assembler.h#newcode97
src/macro-assembler.h:97: bool enter_frame = true)
I don't like this default argument. Rethinking this I think we should avoid
making FrameAndConstantPoolScope a subclass of FrameScope.  It doesn't buy us
much and there are subtle ordering constraints (like set_constant_pool_available
having to happen before masm->EnterFrame and after LeaveFrame) which are
difficult to ensure with this class inheritance.

[rmcilroy, 2014-09-26 15:55:40]

On 2014/09/26 15:54:58, rmcilroy wrote:
> https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h
> File src/ia32/assembler-ia32.h (right):
> 
>
https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
> src/ia32/assembler-ia32.h:1102: // Support not implemented in ia32 yet.
> We will probably never us a constant pool on ia32, so you can just remove the
> comment and UNREACHABLE and always return false here.
> 
>
https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
> src/ia32/assembler-ia32.h:1115: bool is_const_pool_blocked() const {
> You don't need this function (that is only for the inline constant pool and
will
> be going away on Arm once we switch to OOL constant pool).
> 
>
https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
> src/ia32/assembler-ia32.h:1122: // Support not implemented in ia32 yet.
> replace comment with: "// Constant pool is not supported on ia32"
> 
>
https://codereview.chromium.org/609843002/diff/1/src/ia32/macro-assembler-ia3...
> File src/ia32/macro-assembler-ia32.cc (right):
> 
>
https://codereview.chromium.org/609843002/diff/1/src/ia32/macro-assembler-ia3...
> src/ia32/macro-assembler-ia32.cc:902: bool load_constant_pool) {
> Add an ASSERT that load_constant_pool is always false.  Also could you rename
> the variable (both here and on arm) to load_constant_pool_pointer_reg please.
> 
> https://codereview.chromium.org/609843002/diff/1/src/macro-assembler.h
> File src/macro-assembler.h (right):
> 
>
https://codereview.chromium.org/609843002/diff/1/src/macro-assembler.h#newcode97
> src/macro-assembler.h:97: bool enter_frame = true)
> I don't like this default argument. Rethinking this I think we should avoid
> making FrameAndConstantPoolScope a subclass of FrameScope.  It doesn't buy us
> much and there are subtle ordering constraints (like
set_constant_pool_available
> having to happen before masm->EnterFrame and after LeaveFrame) which are
> difficult to ensure with this class inheritance.

Could you also make the description and CL title more descriptive please
(mentioning this is re-factoring to enable it to be used by other
architectures).

[baixo, 2014-10-02 09:18:21]

https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h
File src/ia32/assembler-ia32.h (right):

https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:1102: // Support not implemented in ia32 yet.
On 2014/09/26 15:54:57, rmcilroy wrote:
> We will probably never us a constant pool on ia32, so you can just remove the
> comment and UNREACHABLE and always return false here.

Done.

https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:1115: bool is_const_pool_blocked() const {
On 2014/09/26 15:54:57, rmcilroy wrote:
> You don't need this function (that is only for the inline constant pool and
will
> be going away on Arm once we switch to OOL constant pool).

Done.

https://codereview.chromium.org/609843002/diff/1/src/ia32/assembler-ia32.h#ne...
src/ia32/assembler-ia32.h:1122: // Support not implemented in ia32 yet.
On 2014/09/26 15:54:57, rmcilroy wrote:
> replace comment with: "// Constant pool is not supported on ia32"

Done.

https://codereview.chromium.org/609843002/diff/1/src/ia32/macro-assembler-ia3...
File src/ia32/macro-assembler-ia32.cc (right):

https://codereview.chromium.org/609843002/diff/1/src/ia32/macro-assembler-ia3...
src/ia32/macro-assembler-ia32.cc:902: bool load_constant_pool) {
On 2014/09/26 15:54:57, rmcilroy wrote:
> Add an ASSERT that load_constant_pool is always false.  Also could you rename
> the variable (both here and on arm) to load_constant_pool_pointer_reg please.

Done.

https://codereview.chromium.org/609843002/diff/1/src/macro-assembler.h
File src/macro-assembler.h (right):

https://codereview.chromium.org/609843002/diff/1/src/macro-assembler.h#newcode97
src/macro-assembler.h:97: bool enter_frame = true)
On 2014/09/26 15:54:57, rmcilroy wrote:
> I don't like this default argument. Rethinking this I think we should avoid
> making FrameAndConstantPoolScope a subclass of FrameScope.  It doesn't buy us
> much and there are subtle ordering constraints (like
set_constant_pool_available
> having to happen before masm->EnterFrame and after LeaveFrame) which are
> difficult to ensure with this class inheritance.

I was worried about code duplication, since this class is very similar to
FrameScope. But yes, the subclass approach will only work if the ordering
changes are not an issue. I reverted back to the original
FrameAndConstantPoolScope class.

[rmcilroy, 2014-10-02 15:01:38]

https://codereview.chromium.org/609843002/diff/20001/src/arm64/assembler-arm64.h
File src/arm64/assembler-arm64.h (right):

https://codereview.chromium.org/609843002/diff/20001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.h:1903: 
I think we should put both  is_constant_pool_available() and
set_constant_pool_available(bool available) in BaseAssembler instead rather than
copying to all architectures. Could you rename to is_ool_constant_pool_available
/ set_ool_constant_pool_available to be clear then make the code:

 bool is_ool_constant_pool_available() const {
   if (FLAG_enable_ool_constant_pool) {
     return ool_constant_pool_available_;
   } else {
     // Out-of-line constant pool not supported on this architecture.
     UNREACHABLE();
     return false;
   }
 }

and similar for set_constant_pool_available.

https://codereview.chromium.org/609843002/diff/20001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.h:2026: 
nit - remove extra newline.

https://codereview.chromium.org/609843002/diff/20001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.h:2264: // constant pool
These should move to BaseAssembler if you
is_constant_pool_available/set_constant_pool_available there as in my comment
above.

https://codereview.chromium.org/609843002/diff/20001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/609843002/diff/20001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:3069:
DCHECK(!load_constant_pool_pointer_reg);
Rather than adding the unnecessary argument to this function, could you just
keep this one as-is and add a separate function with both args which does the
DCHECK and calls though to this function (with a comment saying the constant
pool isn't supported on this architecture) - for all architectures.

[baixo, 2014-10-09 09:08:14]

Addressed Ross' comments.

https://codereview.chromium.org/609843002/diff/20001/src/arm64/assembler-arm64.h
File src/arm64/assembler-arm64.h (right):

https://codereview.chromium.org/609843002/diff/20001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.h:1903: 
On 2014/10/02 15:01:37, rmcilroy wrote:
> I think we should put both  is_constant_pool_available() and
> set_constant_pool_available(bool available) in BaseAssembler instead rather
than
> copying to all architectures. Could you rename to
is_ool_constant_pool_available
> / set_ool_constant_pool_available to be clear then make the code:
> 
>  bool is_ool_constant_pool_available() const {
>    if (FLAG_enable_ool_constant_pool) {
>      return ool_constant_pool_available_;
>    } else {
>      // Out-of-line constant pool not supported on this architecture.
>      UNREACHABLE();
>      return false;
>    }
>  }
> 
> and similar for set_constant_pool_available.
> 

Done.

https://codereview.chromium.org/609843002/diff/20001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.h:2026: 
On 2014/10/02 15:01:37, rmcilroy wrote:
> nit - remove extra newline.

Done.

https://codereview.chromium.org/609843002/diff/20001/src/arm64/assembler-arm6...
src/arm64/assembler-arm64.h:2264: // constant pool
On 2014/10/02 15:01:37, rmcilroy wrote:
> These should move to BaseAssembler if you
> is_constant_pool_available/set_constant_pool_available there as in my comment
> above.

Done.

https://codereview.chromium.org/609843002/diff/20001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/609843002/diff/20001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:3069:
DCHECK(!load_constant_pool_pointer_reg);
On 2014/10/02 15:01:38, rmcilroy wrote:
> Rather than adding the unnecessary argument to this function, could you just
> keep this one as-is and add a separate function with both args which does the
> DCHECK and calls though to this function (with a comment saying the constant
> pool isn't supported on this architecture) - for all architectures.

Done.

[rmcilroy, 2014-10-09 09:22:47]

rmcilroy@chromium.org changed reviewers:
+ ulan@chromium.org

[rmcilroy, 2014-10-09 09:22:48]

lgtm once comments are addressed.

Ulan do you want to take a look?

https://codereview.chromium.org/609843002/diff/40001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/609843002/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:3069: // Constant pool not implemented on
arm64.
Out-of-line constant pool not... (arm64 does have an inline constant pool). 
Please change for other architectures as well.

https://codereview.chromium.org/609843002/diff/40001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/609843002/diff/40001/src/assembler.cc#newcode135
src/assembler.cc:135:
ool_constant_pool_available_(!FLAG_enable_ool_constant_pool) {
nit - just make this false (we were previously setting it to true if
!FLAG_enable_ool_constant_pool on Arm since we were using it to check for inline
constant pool too).

https://codereview.chromium.org/609843002/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/609843002/diff/40001/src/assembler.h#newcode135
src/assembler.h:135: // constant pool
/s/constant pool/Constant pool.

[rmcilroy, 2014-10-09 10:12:08]

+simonb

[baixo, 2014-10-09 11:21:38]

Addressed Ross' comments.

https://codereview.chromium.org/609843002/diff/40001/src/arm64/macro-assemble...
File src/arm64/macro-assembler-arm64.cc (right):

https://codereview.chromium.org/609843002/diff/40001/src/arm64/macro-assemble...
src/arm64/macro-assembler-arm64.cc:3069: // Constant pool not implemented on
arm64.
On 2014/10/09 09:22:48, rmcilroy wrote:
> Out-of-line constant pool not... (arm64 does have an inline constant pool). 
> Please change for other architectures as well.

Done.

https://codereview.chromium.org/609843002/diff/40001/src/assembler.cc
File src/assembler.cc (right):

https://codereview.chromium.org/609843002/diff/40001/src/assembler.cc#newcode135
src/assembler.cc:135:
ool_constant_pool_available_(!FLAG_enable_ool_constant_pool) {
On 2014/10/09 09:22:48, rmcilroy wrote:
> nit - just make this false (we were previously setting it to true if
> !FLAG_enable_ool_constant_pool on Arm since we were using it to check for
inline
> constant pool too).

Done.

https://codereview.chromium.org/609843002/diff/40001/src/assembler.h
File src/assembler.h (right):

https://codereview.chromium.org/609843002/diff/40001/src/assembler.h#newcode135
src/assembler.h:135: // constant pool
On 2014/10/09 09:22:48, rmcilroy wrote:
> /s/constant pool/Constant pool.

Done.

[rmcilroy, 2014-10-13 14:41:52]

Committed patchset #5 (id:80001) manually as 24565 (presubmit successful).