OdinMonkey: expose alignment masks to GVN

RESOLVED FIXED in Firefox 39

Status

()

--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla39
x86_64
All
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
Created attachment 8570285 [details] [diff] [review]
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.
Attachment #8570285 - Flags: feedback?(luke)
(Assignee)

Updated

4 years ago
Attachment #8570285 - Flags: feedback?(luke) → review?(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.
Attachment #8570285 - Flags: review?(luke) → review+
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)
(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?
(Assignee)

Comment 4

4 years ago
Created attachment 8570532 [details] [diff] [review]
multiple-uses.patch

Here's the patch which removes the hasOneUse() restriction on the MAdd.
Attachment #8570532 - Flags: review?(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!
Attachment #8570532 - Flags: review?(luke) → review+
https://hg.mozilla.org/mozilla-central/rev/5def1d193a0c
https://hg.mozilla.org/mozilla-central/rev/4c9ebb3591c4
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.