New bugfix for phi eliminator: if a loop phi is used after its update, the load of the phi must n...

New bugfix for phi eliminator: if a loop phi is used after its update, the load of the phi must not be generated at use site. Otherwise, the users would use the updated value, instead of the old value.

[kasperl, 2011-12-22 15:11:54]

LGTM with comment:

http://codereview.chromium.org/9025012/diff/1001/dart/frog/leg/ssa/phi_elimin...
File dart/frog/leg/ssa/phi_eliminator.dart (right):

http://codereview.chromium.org/9025012/diff/1001/dart/frog/leg/ssa/phi_elimin...
dart/frog/leg/ssa/phi_eliminator.dart:130: !isUsedAfterStores(stores[1], load))
{
Is stores[1] guaranteed to be the last store. I guess so. Maybe pass all the
stores to the isUsedAfterStores and let that function deal with it. That makes
it easier to add a nice big comment that explains why you're using stores[1],
not stores.last(), or stores[3.14].

[ngeoffray, 2011-12-22 15:15:10]

Thanks!

http://codereview.chromium.org/9025012/diff/1001/dart/frog/leg/ssa/phi_elimin...
File dart/frog/leg/ssa/phi_eliminator.dart (right):

http://codereview.chromium.org/9025012/diff/1001/dart/frog/leg/ssa/phi_elimin...
dart/frog/leg/ssa/phi_eliminator.dart:130: !isUsedAfterStores(stores[1], load))
{
On 2011/12/22 15:11:54, kasperl wrote:
> Is stores[1] guaranteed to be the last store. I guess so. Maybe pass all the
> stores to the isUsedAfterStores and let that function deal with it. That makes
> it easier to add a nice big comment that explains why you're using stores[1],
> not stores.last(), or stores[3.14].

On a loop phi we currently have the assumption that there are only two
predecessors: the entry (input 0), and the back edge (input 1). I added a
comment and an assert.

[ngeoffray, 2011-12-22 15:58:57]

PTAL, this new change keeps the current 'optimized' (read prettier code) version
that we were generating on our current test cases.

[floitsch, 2011-12-22 16:30:56]

FYI.

http://codereview.chromium.org/9025012/diff/5003/dart/frog/leg/ssa/phi_elimin...
File dart/frog/leg/ssa/phi_eliminator.dart (right):

http://codereview.chromium.org/9025012/diff/5003/dart/frog/leg/ssa/phi_elimin...
dart/frog/leg/ssa/phi_eliminator.dart:146: if
(instruction.block.parentLoopHeader != update.block.parentLoopHeader) {
What about nested loops? Then the parentLoopHeader is different, but the load
would still be used inside the loop.

[ngeoffray, 2011-12-27 08:51:03]

On 2011/12/22 16:30:56, floitsch wrote:
> FYI.
> 
>
http://codereview.chromium.org/9025012/diff/5003/dart/frog/leg/ssa/phi_elimin...
> File dart/frog/leg/ssa/phi_eliminator.dart (right):
> 
>
http://codereview.chromium.org/9025012/diff/5003/dart/frog/leg/ssa/phi_elimin...
> dart/frog/leg/ssa/phi_eliminator.dart:146: if
> (instruction.block.parentLoopHeader != update.block.parentLoopHeader) {
> What about nested loops? Then the parentLoopHeader is different, but the load
> would still be used inside the loop.

You are right. I am closing this CL for another CL that disables generate at use
site for loop phis.