Boris Zbarsky [:bzbarsky] Reporter
	Description • 12 years ago

In the attached testcase, we end up unable to hoist the img.data get in the if condition because it thinks it's aliased by some setter in the loop.  And the reason it thinks that is that we have no type info for the if body, so the img.data[0] get there produces MCallGetElement which aliases the world.

	Dan Gohman [:sunfish]
	Comment 1 • 10 years ago

Here's a version of the testcase which can be run in a standalone JS shell and which is slightly more complex to prevent Ion from finding ways around the problem. The "t += img.data[0]" never executes, so we don't get a type specialization for it, so MBinaryArithInstruction::getAliasSet calls it AliasSet::Store(AliasSet::Any), and this blocks LICM of all loads in the loop.

	Jan de Mooij [:jandem]
	Comment 2 • 10 years ago

I think the main problem here is that we often add MIR instructions that we know will always bail when they execute (empty type barriers, say) and then go on to add even more instructions to those blocks. We can potentially waste a lot of time optimizing dead code.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 9 years ago

This seems to be fixed now.  Did we start ignoring branches with no type info for LICM purposes at some point?

	Jan de Mooij [:jandem]
	Comment 4 • 9 years ago

(In reply to Boris Zbarsky [:bz] from comment #3)
> This seems to be fixed now.  Did we start ignoring branches with no type
> info for LICM purposes at some point?

This is the loop body:

  if (img.data[0] == 1) {
    t += img.data[0];
  }

TI knows (and we insert a TypeBarrier) that the first img.data[0] is |undefined|, so the if-expression becomes:

  if (undefined == 1)

GVN knows this is always false, so it's replaced with an MGoto. Then it notices the if-body is now unreachable so we remove it. After that, LICM is trivial.

So no, we didn't fix the real issue here, but nbp is working on pruning cold branches based on Baseline feedback, this should also get rid of the if-body before we do LICM (and it should work in more cases).

	Guilherme Lima
	Comment 5 • 8 years ago

Fixed by bug 1229813?

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 6 • 8 years ago

Should be, yes.