ia32: Remove redundant CTX preservation from intrinsics; tweak definition of CTX.

ia32: Remove redundant CTX preservation from intrinsics; tweak definition of CTX.

Redefines CTX from ESI to EDI only to demonstrate that no assumptions of its specific value exist in the current code.

R=fschneider@google.com, regis@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44699

[regis, 2015-03-24 22:36:16]

LGTM

However, I have lost track of the usage of register CTX vs  local variable
:current_context_var.

Florian, it would be nice to have a comment next to the declaration of CTX
explaining this.

Thanks,
Regis

[Florian Schneider, 2015-03-25 08:21:27]

lgtm

https://codereview.chromium.org/1005353004/diff/40001/runtime/vm/constants_ia...
File runtime/vm/constants_ia32.h (right):

https://codereview.chromium.org/1005353004/diff/40001/runtime/vm/constants_ia...
runtime/vm/constants_ia32.h:65: const Register CTX = EDI;  // Caches current
context in generated code.
The comment is outdated: The context is kept in a local variable and only starts
in CTX at method entry.

[regis, 2015-03-25 16:05:13]

On 2015/03/25 08:21:27, Florian Schneider wrote:
> lgtm
> 
>
https://codereview.chromium.org/1005353004/diff/40001/runtime/vm/constants_ia...
> File runtime/vm/constants_ia32.h (right):
> 
>
https://codereview.chromium.org/1005353004/diff/40001/runtime/vm/constants_ia...
> runtime/vm/constants_ia32.h:65: const Register CTX = EDI;  // Caches current
> context in generated code.
> The comment is outdated: The context is kept in a local variable and only
starts
> in CTX at method entry.

Thanks.
I suppose that CTX is not reserved globally anymore? I think it would make sense
to remove its declaration altogether from all constants-<arch>.h headers since
it is not a dedicated register anymore.

If the context is passed as an additional argument to closures, maybe it should
be treated as such. The variable :current_context_var could be a formal
parameter. We already pass hidden parameters to factories and constructors. Of
course, this is not a change for this cl.

[Florian Schneider, 2015-03-25 16:07:13]

On 2015/03/25 16:05:13, regis wrote:
> On 2015/03/25 08:21:27, Florian Schneider wrote:
> > lgtm
> > 
> >
>
https://codereview.chromium.org/1005353004/diff/40001/runtime/vm/constants_ia...
> > File runtime/vm/constants_ia32.h (right):
> > 
> >
>
https://codereview.chromium.org/1005353004/diff/40001/runtime/vm/constants_ia...
> > runtime/vm/constants_ia32.h:65: const Register CTX = EDI;  // Caches current
> > context in generated code.
> > The comment is outdated: The context is kept in a local variable and only
> starts
> > in CTX at method entry.
> 
> Thanks.
> I suppose that CTX is not reserved globally anymore? I think it would make
sense
> to remove its declaration altogether from all constants-<arch>.h headers since
> it is not a dedicated register anymore.
> 
> If the context is passed as an additional argument to closures, maybe it
should
> be treated as such. The variable :current_context_var could be a formal
> parameter. We already pass hidden parameters to factories and constructors. Of
> course, this is not a change for this cl.

Yes, but the flow graph allocator still needs a fixed register where the context
starts out at method entry and this is in platform-independent code.

So we probably need a platform-independent name for this register.

[koda, 2015-03-25 16:10:53]

https://codereview.chromium.org/1005353004/diff/40001/runtime/vm/constants_ia...
File runtime/vm/constants_ia32.h (right):

https://codereview.chromium.org/1005353004/diff/40001/runtime/vm/constants_ia...
runtime/vm/constants_ia32.h:65: const Register CTX = EDI;  // Caches current
context in generated code.
On 2015/03/25 08:21:27, Florian Schneider wrote:
> The comment is outdated: The context is kept in a local variable and only
starts
> in CTX at method entry.

Updated the comment, for all archs.

[koda, 2015-03-25 21:25:44]

Committed patchset #4 (id:60001) manually as r44699 (presubmit successful).