[vm] Cleanup Instruction::Effects(), prepare to cleanup Dependencies()

[vm] Cleanup Instruction::Effects(), prepare to cleanup Dependencies()

This CL continues cleanup after Dart_MakeExternalString API was removed
in 5b694c1b1ca4a17e5aad865c6c4feb391f1e2b66.

* Replace Instruction::Effects() with Instruction::HasSideEffects().

* Prepare to clean up Instruction::Dependencies() - assert that it is
  used only if AllowsCSE(). In such case it always returns
  EffectSet::None().

* Clean up 2 separate maps for instructions with dependencies and
  without dependencies in CSEInstructionMap, as it only works on
  instructions with AllowsCSE() and, as a consequence, without
  dependencies.

* Clean up code which was testing Dependencies() of CheckClassInstr
  and CheckClassIdInstr.

* Fix LoadStaticFieldInstr::AllowsCSE() to take FLAG_fields_may_be_reset
  into account - like in LoadStaticFieldInstr::Dependencies().

R=vegorov@google.com

Issue: https://github.com/dart-lang/sdk/issues/30474
Committed: https://github.com/dart-lang/sdk/commit/36140a7519ba122a2d81b1731902dbf535ba283c

[alexmarkov, 2017-08-21 21:08:36]

alexmarkov@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com, vegorov@google.com

[Vyacheslav Egorov (Google), 2017-08-22 14:08:39]

lgtm

https://codereview.chromium.org/3003593002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/3003593002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:877: virtual bool HasSideEffects() const = 0;
I would give it a name like HasUnknownSideEffects() or something similar.

The reason being is that some side-effectfull instructions (e.g. stores) return
false from this function.

Maybe also reflect this in the comment above the function: e.g. "if this
instruction has any side-effects besides storing. See
StoreInstanceFieldInstr::Has...Effect() for rationale."

[alexmarkov, 2017-08-22 15:51:57]

https://codereview.chromium.org/3003593002/diff/20001/runtime/vm/intermediate...
File runtime/vm/intermediate_language.h (right):

https://codereview.chromium.org/3003593002/diff/20001/runtime/vm/intermediate...
runtime/vm/intermediate_language.h:877: virtual bool HasSideEffects() const = 0;
On 2017/08/22 14:08:39, Vyacheslav Egorov (Google) wrote:
> I would give it a name like HasUnknownSideEffects() or something similar.
> 
> The reason being is that some side-effectfull instructions (e.g. stores)
return
> false from this function.
> 
> Maybe also reflect this in the comment above the function: e.g. "if this
> instruction has any side-effects besides storing. See
> StoreInstanceFieldInstr::Has...Effect() for rationale."
> 
>   

Done.

[alexmarkov, 2017-08-22 15:53:36]

Description was changed from

==========
[vm] Cleanup Instruction::Effects(), prepare to cleanup Dependencies()

This CL continues cleanup after Dart_MakeExternalString API was removed
in 5b694c1b1ca4a17e5aad865c6c4feb391f1e2b66.

* Replace Instruction::Effects() with Instruction::HasSideEffects().

* Prepare to clean up Instruction::Dependencies() - assert that it is
  used only if AllowsCSE(). In such case it always returns
  EffectSet::None().

* Clean up 2 separate maps for instructions with dependencies and
  without dependencies in CSEInstructionMap, as it only works on
  instructions with AllowsCSE() and, as a consequence, without
  dependencies.

* Clean up code which was testing Dependencies() of CheckClassInstr
  and CheckClassIdInstr.

* Fix LoadStaticFieldInstr::AllowsCSE() to take FLAG_fields_may_be_reset
  into account - like in LoadStaticFieldInstr::Dependencies().

Issue: https://github.com/dart-lang/sdk/issues/30474
==========

to

==========
[vm] Cleanup Instruction::Effects(), prepare to cleanup Dependencies()

This CL continues cleanup after Dart_MakeExternalString API was removed
in 5b694c1b1ca4a17e5aad865c6c4feb391f1e2b66.

* Replace Instruction::Effects() with Instruction::HasSideEffects().

* Prepare to clean up Instruction::Dependencies() - assert that it is
  used only if AllowsCSE(). In such case it always returns
  EffectSet::None().

* Clean up 2 separate maps for instructions with dependencies and
  without dependencies in CSEInstructionMap, as it only works on
  instructions with AllowsCSE() and, as a consequence, without
  dependencies.

* Clean up code which was testing Dependencies() of CheckClassInstr
  and CheckClassIdInstr.

* Fix LoadStaticFieldInstr::AllowsCSE() to take FLAG_fields_may_be_reset
  into account - like in LoadStaticFieldInstr::Dependencies().

R=vegorov@google.com

Issue: https://github.com/dart-lang/sdk/issues/30474
Committed:
https://github.com/dart-lang/sdk/commit/36140a7519ba122a2d81b1731902dbf535ba283c
==========

[alexmarkov, 2017-08-22 15:53:38]

Committed patchset #3 (id:40001) manually as
36140a7519ba122a2d81b1731902dbf535ba283c (presubmit successful).