ARM/ARM64: Fix smashed CODE_REG in intrinsics with InvokeMathCFunctionInstrs.

ARM/ARM64: Fix smashed CODE_REG in intrinsics with InvokeMathCFunctionInstrs.

X64: Fix smashed ARGS_DESC_REG.

IA32: Remove unnecessary preservation of ICREG.

BUG=http://dartbug.com/26516
R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/df4e0d5ac6031df1a11fa84fe4724ca5ebb3df10

[rmacnak, 2016-05-24 22:08:19]

Description was changed from

==========
ARM/ARM64: Fix smashed CODE_REG in intrinsics with InvokeMathCFunctionInstrs.

IA32: Remove unnecessary preservation of ICREG.

BUG=http://dartbug.com/26516
==========

to

==========
ARM/ARM64: Fix smashed CODE_REG in intrinsics with InvokeMathCFunctionInstrs.

X64: Fix smashed ARGS_DESC_REG.

IA32: Remove unnecessary preservation of ICREG.

BUG=http://dartbug.com/26516
==========

[rmacnak, 2016-05-24 22:18:39]

rmacnak@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2016-05-25 11:15:32]

lgtm

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/inter...
File runtime/vm/intermediate_language_x64.cc (right):

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/inter...
runtime/vm/intermediate_language_x64.cc:5190: result->set_temp(0,
Location::RegisterLocation(R13));
This needs an assertion somewhere that R13 is callee save register.

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
File runtime/vm/intrinsifier_arm.cc (right):

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
runtime/vm/intrinsifier_arm.cc:37: COMPILE_ASSERT((1 << CODE_REG) &
kAbiPreservedCpuRegs);
(...) != 0

our style does not allow implicit integral to boolean coercion - even though it
allows to write concise and still readable code.

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
File runtime/vm/intrinsifier_ia32.cc (left):

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
runtime/vm/intrinsifier_ia32.cc:45: assembler->movl(CALLEE_SAVED_TEMP, ICREG);
I wonder why this was preserved in the first place. I can't find any uses in the
prologue.

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
File runtime/vm/intrinsifier_mips.cc (right):

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
runtime/vm/intrinsifier_mips.cc:43: assembler->mov(CALLEE_SAVED_TEMP, RA);
RA -> LRREG

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/runti...
File runtime/vm/runtime_entry_arm64.cc (right):

https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/runti...
runtime/vm/runtime_entry_arm64.cc:55: COMPILE_ASSERT(R23 != CODE_REG);
It might be good to add a comment here why this matters.

In general I think we need a better way of blocking registers to catch places
like this where we use arbitrary registers, but this is a question for another
refactoring. 

Maybe we should teach graph intensifier to do the magic himself.

[Chinmay, 2016-05-25 16:50:13]

On 2016/05/25 at 11:15:32, vegorov wrote:
> lgtm
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/inter...
> File runtime/vm/intermediate_language_x64.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/inter...
> runtime/vm/intermediate_language_x64.cc:5190: result->set_temp(0,
Location::RegisterLocation(R13));
> This needs an assertion somewhere that R13 is callee save register.
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
> File runtime/vm/intrinsifier_arm.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
> runtime/vm/intrinsifier_arm.cc:37: COMPILE_ASSERT((1 << CODE_REG) &
kAbiPreservedCpuRegs);
> (...) != 0
> 
> our style does not allow implicit integral to boolean coercion - even though
it allows to write concise and still readable code.
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
> File runtime/vm/intrinsifier_ia32.cc (left):
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
> runtime/vm/intrinsifier_ia32.cc:45: assembler->movl(CALLEE_SAVED_TEMP, ICREG);
> I wonder why this was preserved in the first place. I can't find any uses in
the prologue.
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
> File runtime/vm/intrinsifier_mips.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/intri...
> runtime/vm/intrinsifier_mips.cc:43: assembler->mov(CALLEE_SAVED_TEMP, RA);
> RA -> LRREG
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/runti...
> File runtime/vm/runtime_entry_arm64.cc (right):
> 
>
https://chromiumcodereview.appspot.com/2003403003/diff/60001/runtime/vm/runti...
> runtime/vm/runtime_entry_arm64.cc:55: COMPILE_ASSERT(R23 != CODE_REG);
> It might be good to add a comment here why this matters.
> 
> In general I think we need a better way of blocking registers to catch places
like this where we use arbitrary registers, but this is a question for another
refactoring. 
> 
> Maybe we should teach graph intensifier to do the magic himself.

FWIW, I applied this patch locally and was able to verify that all outstanding
issues related to this were fixed. Do let me know when you think its good for me
to roll forward. Will verify again and close relevant tickets.

Thanks!

[rmacnak, 2016-05-25 16:58:52]

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/intermediate...
File runtime/vm/intermediate_language_x64.cc (right):

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/intermediate...
runtime/vm/intermediate_language_x64.cc:5190: result->set_temp(0,
Location::RegisterLocation(R13));
On 2016/05/25 11:15:31, Vyacheslav Egorov (Google) wrote:
> This needs an assertion somewhere that R13 is callee save register.

Done.

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_arm.cc (right):

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_arm.cc:37: COMPILE_ASSERT((1 << CODE_REG) &
kAbiPreservedCpuRegs);
On 2016/05/25 11:15:31, Vyacheslav Egorov (Google) wrote:
> (...) != 0
> 
> our style does not allow implicit integral to boolean coercion - even though
it
> allows to write concise and still readable code.

Factored out as IsABIPreservedRegister.

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_ia32.cc (left):

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_ia32.cc:45: assembler->movl(CALLEE_SAVED_TEMP, ICREG);
On 2016/05/25 11:15:31, Vyacheslav Egorov (Google) wrote:
> I wonder why this was preserved in the first place. I can't find any uses in
the
> prologue.

I suspect it was for something in the past. I previously removed it from
ParallelMoveResolver when adding support for the iOS ARM ABI.

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/intrinsifier...
File runtime/vm/intrinsifier_mips.cc (right):

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/intrinsifier...
runtime/vm/intrinsifier_mips.cc:43: assembler->mov(CALLEE_SAVED_TEMP, RA);
On 2016/05/25 11:15:31, Vyacheslav Egorov (Google) wrote:
> RA -> LRREG

Done.

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/runtime_entr...
File runtime/vm/runtime_entry_arm64.cc (right):

https://codereview.chromium.org/2003403003/diff/60001/runtime/vm/runtime_entr...
runtime/vm/runtime_entry_arm64.cc:55: COMPILE_ASSERT(R23 != CODE_REG);
On 2016/05/25 11:15:31, Vyacheslav Egorov (Google) wrote:
> It might be good to add a comment here why this matters.

Done.
 
> In general I think we need a better way of blocking registers to catch places
> like this where we use arbitrary registers, but this is a question for another
> refactoring. 
> 
> Maybe we should teach graph intensifier to do the magic himself.

[rmacnak, 2016-05-25 17:02:25]

Description was changed from

==========
ARM/ARM64: Fix smashed CODE_REG in intrinsics with InvokeMathCFunctionInstrs.

X64: Fix smashed ARGS_DESC_REG.

IA32: Remove unnecessary preservation of ICREG.

BUG=http://dartbug.com/26516
==========

to

==========
ARM/ARM64: Fix smashed CODE_REG in intrinsics with InvokeMathCFunctionInstrs.

X64: Fix smashed ARGS_DESC_REG.

IA32: Remove unnecessary preservation of ICREG.

BUG=http://dartbug.com/26516
R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/df4e0d5ac6031df1a11fa84fe4724ca5ebb3df10
==========

[rmacnak, 2016-05-25 17:02:26]

Committed patchset #6 (id:100001) manually as
df4e0d5ac6031df1a11fa84fe4724ca5ebb3df10 (presubmit successful).