1137573 - OdinMonkey: expose alignment masks to GVN

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: alignment-mask-analysis.patch

This patch rewrites (a+i)&m to (a&m)+i when it's likely that a&m will become redundant, and possibly also that i can be folded as a constant offset. It does so before GVN to expose the redundancy to optimization.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8570285 [details] [diff] [review]
alignment-mask-analysis.patch

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

Beautiful!

::: js/src/jit/AlignmentMaskAnalysis.cpp
@@ +46,2 @@
>      }
> +    if (!lhs->isAdd() || !lhs->hasOneUse() || !rhs->isConstantValue())

From irc, I eagerly await the patch that removes hasOneUse.

::: js/src/jit/IonOptimizationLevels.cpp
@@ +23,5 @@
>  {
>      level_ = Optimization_Normal;
>  
>      eaa_ = true;
> +    ama_ = true;

EAA can help normal Ion b/c it folds normal arith into MEffectiveAddress, so it deserves to be enabled in initNormalOptimizationInfo, but AMA isn't, so I think it should go into initAsmjsOptimizationInfo.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8570285 [details] [diff] [review]
alignment-mask-analysis.patch

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

Nice!

::: js/src/jit/AlignmentMaskAnalysis.cpp
@@ +46,2 @@
>      }
> +    if (!lhs->isAdd() || !lhs->hasOneUse() || !rhs->isConstantValue())

I guess this restriction is due to the fact we're replacing one operand of the add below? Could we instead clone the add (GVN happens later)? Does it actually happen in practice a lot that lhs has more than one use at this point? (IonBuilder tends to duplicate MIR nodes most of the time, and GVN happens later, so i wonder if this really is a problem)

Comment 3

[Douglas Crosher [:dougc]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #2)
> Comment on attachment 8570285 [details] [diff] [review]
> alignment-mask-analysis.patch
> 
> Review of attachment 8570285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice!
> 
> ::: js/src/jit/AlignmentMaskAnalysis.cpp
> @@ +46,2 @@
> >      }
> > +    if (!lhs->isAdd() || !lhs->hasOneUse() || !rhs->isConstantValue())
> 
> I guess this restriction is due to the fact we're replacing one operand of
> the add below? Could we instead clone the add (GVN happens later)? Does it
> actually happen in practice a lot that lhs has more than one use at this
> point? (IonBuilder tends to duplicate MIR nodes most of the time, and GVN
> happens later, so i wonder if this really is a problem)

Yes, it's very common to see these (i+c) computations hoisted in Emscripten output and thus to have multiple uses. Could it be handled by creating a new op rather than destructively modifying the op when it has more uses?

Comment 4

[Dan Gohman [:sunfish]]

Attachment: multiple-uses.patch

Here's the patch which removes the hasOneUse() restriction on the MAdd.

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 8570532 [details] [diff] [review]
multiple-uses.patch

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

Simpler, more general, and at the cost of only a bit more node allocation.  Nice!

Comment 6

[Dan Gohman [:sunfish]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5def1d193a0c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4c9ebb3591c4

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5def1d193a0c
https://hg.mozilla.org/mozilla-central/rev/4c9ebb3591c4