869501 - Missed opportunities to form CompareDAndBranch

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Dan Gohman [:sunfish]]

In js/src/ion/Lowering.cpp, the optimization to form LCompareDAndBranch nodes instead of doing a separate comparison and branch checks for a compareType() of Compare_Double. This means it misses cases that use Compare_DoubleMaybeCoerceLHS and Compare_DoubleMaybeCoerceRHS. This causes it to sometimes generate code like this:

  ucomisd %xmm1, %xmm0
  seta    %cl
  movzbl  %ecx, %ecx
  [...]
  testl   %ecx, %ecx
  jne     [...]

for Javascript code like this (warning, contrived testcase):

var array = [ 0.1, undefined ];

for (var i = 0; i != array.length; ++i)
  if (array[i] < 0)
    print('hi');

Comment 1

[Dan Gohman [:sunfish]]

Attachment: a patch containing a proposed fix

This patch adds checks for the MaybeCoerce forms of Compare_Double, which is consistent with the code later on which forms LCompareD nodes. I'm still new to this code, so I don't know if there are reasons for not doing this.

Comment 2

[Hannes Verschore [:h4writer]]

Thanks! I've added Brain, since he introduced Compare_DoubleMaybeCoerceLHS recently. I don't know the real objective of that actually. But I saw it passing on tbpl.

If you think it is finished you can always request a review to the right people. Usually you can find this by using "hg annotate -u" on the changed file and see who changed that particular line before. In this case I would recommend bhackett ;)

And feel free to ask additional questions here or on the irc channel (#jsapi and/or #ionmonkey)

Comment 3

[Brian Hackett [Laid off!]]

Good catch, looks like I only updated LIRGenerator::visitCompare for the new comparisons.  The Compare_DoubleMaybeCoerce variants can be treated the same as Compare_Double in lowering and later phases, the coerce just indicates to the type analysis (which runs after the MIR is initially constructed) whether operands may be converted to doubles without bailing out.

Comment 4

[Dan Gohman [:sunfish]]

Would it make sense to rewrite Compare_DoubleMaybeCoerce to Compare_Double once it is no longer needed?

Comment 5

[Brian Hackett [Laid off!]]

Hmm, I think it would be a bit cleaner to have an IsDoubleComparison() static method or something, or remove the Coerce variants and add a couple bits of data to the type policy.

Comment 6

[Dan Gohman [:sunfish]]

Attachment: a revised patch, per feedback

It wasn't clear to me how to add bytes to the type, so I created an isDoubleComparison() method and made both places use it.

Comment 7

[Brian Hackett [Laid off!]]

Comment on attachment 746782 [details] [diff] [review]
a revised patch, per feedback

Review of attachment 746782 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cea585acd21

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6cea585acd21