Added def-use chain to the intermediate language.

Added def-use chain to the intermediate language.


R=fschneider@google.com,kmillikin@google.com
BUG=

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

[zerny-google, 2012-08-09 17:42:11]

On 2012/08/09 17:40:05, zerny wrote:

Will clean the virtual RemoveFromDefUseChain once Slava has committed some
changes to the IR computations.

[srdjan, 2012-08-09 20:07:51]

DBC

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:275: for (intptr_t i = 0; i < N; ++i)
inputs_[i]->RemoveFromDefUseChain();
Can some inputs be NULL? Maybe:

if (inputs_[i] != NULL) {
  inputs_[i]->Remove ...
}

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:334: void set_definition(Definition*
definition);
s/set_definition/SetDefinition/

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:1911: for (intptr_t i = 0; i < N; ++i)
inputs_[i]->RemoveFromDefUseChain();
San some inputs_[i] ne NULL?

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:2330: void set_def_use_chain(UseVal* chain) {
def_use_chain_ = chain; }
Should you assert that chain is head (previous is NULL).

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:2335: UseVal* def_use_chain_;
This is a chain of uses only, isn't it? Maybe rename to use_chain?

[zerny-google, 2012-08-10 08:07:22]

Thanks for the comments.

On 2012/08/09 20:07:51, srdjan wrote:
> DBC
> 
>
https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
> File runtime/vm/intermediate_language.h (right):
> 
>
https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
> runtime/vm/intermediate_language.h:275: for (intptr_t i = 0; i < N; ++i)
> inputs_[i]->RemoveFromDefUseChain();
> Can some inputs be NULL? Maybe:
> 
> if (inputs_[i] != NULL) {
>   inputs_[i]->Remove ...
> }
> 
>
https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
> runtime/vm/intermediate_language.h:334: void set_definition(Definition*
> definition);
> s/set_definition/SetDefinition/
> 
>
https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
> runtime/vm/intermediate_language.h:1911: for (intptr_t i = 0; i < N; ++i)
> inputs_[i]->RemoveFromDefUseChain();
> San some inputs_[i] ne NULL?
> 
>
https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
> runtime/vm/intermediate_language.h:2330: void set_def_use_chain(UseVal* chain)
{
> def_use_chain_ = chain; }
> Should you assert that chain is head (previous is NULL).
> 
>
https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
> runtime/vm/intermediate_language.h:2335: UseVal* def_use_chain_;
> This is a chain of uses only, isn't it? Maybe rename to use_chain?

[zerny-google, 2012-08-10 08:08:29]

Thanks for the comments.

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:275: for (intptr_t i = 0; i < N; ++i)
inputs_[i]->RemoveFromDefUseChain();
On 2012/08/09 20:07:52, srdjan wrote:
> Can some inputs be NULL? Maybe:
> 
> if (inputs_[i] != NULL) {
>   inputs_[i]->Remove ...
> }

Indeed, thanks. It is tempting to assert non-null inputs. Almost all inputs_ are
asserted to be non-null at construction time. The exception to the rule being
PhiInstr. However, when removing a phi (after renaming) its inputs should be
actual values. I'll check this assumption and repair the code accordingly.

A side note: there are a few other instructions that do not assert non-null
inputs, but most likely should: StoreLocalComp, StoreIndexedComp,
BooleanNegateComp, and StoreVMFieldComp.

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:334: void set_definition(Definition*
definition);
On 2012/08/09 20:07:52, srdjan wrote:
> s/set_definition/SetDefinition/

Ok.

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:2330: void set_def_use_chain(UseVal* chain) {
def_use_chain_ = chain; }
On 2012/08/09 20:07:52, srdjan wrote:
> Should you assert that chain is head (previous is NULL).

Sure.

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:2335: UseVal* def_use_chain_;
On 2012/08/09 20:07:52, srdjan wrote:
> This is a chain of uses only, isn't it? Maybe rename to use_chain?

Ok. What about use_list?

[Florian Schneider, 2012-08-10 08:35:37]

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:338: UseVal* next_use() { return next_use_; }
Maybe add const:

UseVal* next_use() const { return next_use_; }

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:339: UseVal* previous_use() { return
previous_use_; }
Const here as well.

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:1137: // TODO(zerny): why are we using an
array here?
You're right, we actually don't need to have an array here.

https://chromiumcodereview.appspot.com/10826230/diff/1/runtime/vm/intermediat...
runtime/vm/intermediate_language.h:1812: void RemoveFromDefUseChain();
This is a declaration without a definition. Is the intention to make it
abstract?

[zerny-google, 2012-08-10 12:17:43]

Improvements to the use-list.

[zerny-google, 2012-08-10 12:20:18]

https://chromiumcodereview.appspot.com/10826230/diff/8001/runtime/vm/intermed...
File runtime/vm/intermediate_language.h (right):

https://chromiumcodereview.appspot.com/10826230/diff/8001/runtime/vm/intermed...
runtime/vm/intermediate_language.h:1821: // Remove uses in this instruction from
the def-use chains.
I'll change this comment.

[Florian Schneider, 2012-08-10 12:28:00]

LGTM.

https://chromiumcodereview.appspot.com/10826230/diff/8001/runtime/vm/intermed...
File runtime/vm/intermediate_language.cc (right):

https://chromiumcodereview.appspot.com/10826230/diff/8001/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:34: 
Add an empty line here.

https://chromiumcodereview.appspot.com/10826230/diff/8001/runtime/vm/intermed...
runtime/vm/intermediate_language.cc:50: 
Add an empty line here.